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

[Owen Taylor <otaylor redhat com>]

Darin Adler <darin bentspoon com> writes:

> 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).

Remember, EelBackground knows nothing about its target drawable --
eel_background_get_desktop_background_window() is in
libnautilus-private. (It may have been a casualty of global
search-and-replace?)

So, barring a fair bit of code reorganization, I'm not quite sure
how to do it. I suppose you could add a function along the lines of:

EelBackground *
eel_get_widget_background_for_use_with_backing_pixmap (GtkWidget *widget)

Keeping a backpointer from EelBackground to the associated widget
might be simpler / easier if you aren't supposed to be able to
use a single EelBackground with multiple windows.

>      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.

If this goes into a new gnome-libs release, I think it would
be best to do as a new interface call and a minor version
bump.

The reason for the g_module hackery is mostly that adding new API in a
Red Hat specific patch is quite dangerous ... if someone upgrades from
our 1.2.13 to, say 1.2.14ximian, then things break without RPM
noticing.

So, I wanted to do things in a manner which was robust against 
mismatches.
 
The canvas-hack part of these patches is definitely excerpts from
Red Hat patches, rather than a final proposal for upstream ...
just there to convey the idea.

>      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.

I don't really have time to look at this right now, but I can
send something to gnome-libs-list quickly laying out the problem.

The hardest part of integrating this cleanly in GnomeCanvas is that
for the desktop background usage, we actually need to use a 
specific Pixmap, created in a special fashion, so it's not
something that can be done automatically by GnomeCanvas.
 
>      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.

g_module itself is portable. g_module_open (NULL, ) and then
accessing symbols shared library is not so portable.
 
>      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.

Well, yes, the get() call could be added. I just had already built
the gnome-libs package, and didn't want to go back and add more
stuff :-)
 
>      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.

The "hack" naming is there to distinguish the current implementation
of the idea from future implemenations done in a clean fashion
which might have incompatible interfaces. The principle of not
treading all over the namespace until you are sure you have
it right.
 
>      7) You should call it the canvas_bg_hack instead of the
> nautilus_bg_hack, I think. But hey, maybe not.

Again the "nautilus" in here is namespacing ... "a hack to use
with nautilus". 
 
>      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.

Good idea.
 
>      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)".

Can be done.
 
> 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.

When I get back from OLS next week, I'll put in the simpler parts
of the patch, and send some mail out about the GnomeCanvas changes.

Regards,
                                        Owen