Re: [Nautilus-list] Desktop background, fixes and hacks

On Monday, July 23, 2001, at 10:58  PM, Owen Taylor wrote:

The two fixes I think should definitely go in the mainline.  I'm less
sure about the hack. It reduces nautilus's share of CPU usage on my
(fast) machine from ~10% to ~1% when moving a window over a otherwise
empty desktop background, so it's a pretty big effect, but the
need to change GnomeCanvas makes things ugly.

I'd like to see all three changes rolled into CVS. The result for the user is more important than a bit of code ugliness, in my opinion. Reducing complaints about slowness is the #1 priority for Nautilus right now.

Here are some thoughts:

1) I'd prefer to see the turn_on_bg_hack() code entirely in eel rather than in Nautilus. One way to do this might be to add an eel call that does the gdk_window_set_back_pixmap() instead of using the current idiom of doing eel_background_get_desktop_background_window() and calling gdk_window_set_back_pixmap(). Then, eel could do the right thing depending on whether the canvas hack is there or not (making a repeated copy of the passed in pixmap if necessary, I guess).

2) I have little influence about new releases of gnome-libs, but I think it would be nice to get this hack in there and see if we can get a new release, since the Nautilus speedup is noticeable.

3) I'd love it if you could get the ball rolling so that GnomeCanvas for Gnome 2.0 has appropriate hooks (if you prefer, the better ones you suggest rather than what you used here) so that we don't have to break the API freeze to get this right for Gnome 2.0.

4) Do you know how portable the use of gmodule is here? I ask because when I used gmodule to find FAM, it turned out to be a portability problem. I don't remember exactly why, but since it had to do with file names, I guess it probably is not an issue here.

5) I don't quite get why you use the gmodule chicanery to turn on the bg hack, but object data with a pre-arranged name to check if the bg hack is on. If the gmodule stuff was all in eel, not Nautilus (see #1 above), and you added a get call as well as a set call, then you could have the object data be entirely private to gnome-libs, which is enough cleaner that I think it's worth it.

6) I personally would use a name which emphasizes the "will use clear_area to draw background if no objects are there" aspect of the hack, to be clear what the flag means, rather than emphasizing the "hack" aspect of it. Putting the emphasis on "hack" is clear on why the code is there, admirably modest, and could even help deflect criticism, but doesn'
t help people understand what the code does.

7) You should call it the canvas_bg_hack instead of the nautilus_bg_hack, I think. But hey, maybe not.

8) I'd like to see a comment in the code that explains the 128 (as you explained it in the email), and perhaps even a #define.

9) Last, but least, the code you added to Nautilus and Eel doesn't follow the Nautilus coding style. I'd be amenable to changing the style guide, since the set of people working on Nautilus has changed, but if we don't , we should change the patch before applying it. Two things I notice is that the patch has code that declares variables in local blocks (something I do in my own code, but the coding style forbids for fairly good reason) and uses "if (x)" rather than "if (x != NULL)".

As long as you consider these points, I'd be happy to see these changes committed to Nautilus and Eel. My approval doesn't mean much for gnome-libs, of course.

    -- Darin

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]