Re: Geometry issues ...

[Michael Meeks <michael ximian com>]

Hi Frank,

On Tue, 2002-07-09 at 20:53, Frank Worsley wrote:
> attached is a little patch that makes Nautilus only save the geometry
> after a timeout. This basically causes the geometry to be saved only
> once for every resize the user performs.

	Hmm; good work - it's better, but I think we need to do more sanity
checking of the geometry before doing the save.

	I think we need:

	a) Store the last saved ConfigureRequest geometry as 
	   x,y,width,height on the NautilusWindow instead of 
	   stringifying it [ at which point it's lost to sane 
	   manipulation ] Better to use a GdkRectangle here IMHO

	b) Only kick off the geometry save timeout if the window is 
	   visible on screen [ not a viewport  window moving hack ],
	   and the geometry is different from the last saved state.

	That will mean that we don't push bogus positioning data to disk
forcing re-renders on switching viewport [ which gives wierd, different
configure requests / geometries ].

> The second part of the patch places new windows at a slightly offset
> position if a window with the same URI is already open, that makes for a
> nicer UI experience. Except ... 

	This should IMHO never be done in user land, it's a really, really bad
move. The Window manager should be in charge of laying out windows - it
really has a lot more information, can do it right, and is not worth
duplicating the code for.

> I agree that hammering the API with constant save requests for every
> drag operation was wrong, the timeout fixes that issue.

	Still too many requests for me - as above, but a lot better.

> And yes, I still think the geometry should be saved in the metadata
> since it belongs to a directory just as much as any other metadata
> like emblems.

	Fine - I'm more concerned about the bugs personally :-)

	Some minor nit picks:

> +	/* Only save the geometry if the user hasn't resized the window
> +	 * for one second. Otherwise delay the callback for another second.

	Here you say 1 second,

> +	if (window->save_geometry_timeout_id == 0) {
> +		window->save_geometry_timeout_id = 
> +				g_timeout_add (500, save_window_geometry_timeout, window);
> +	} else {
> +		g_source_remove (window->save_geometry_timeout_id);
> +		window->save_geometry_timeout_id = 
> +				g_timeout_add (500, save_window_geometry_timeout, window);		

	Here you say 1/2 a second :-) also, instead of cutting and pasting this
line twice, would it not be better to do:

	if (timeout_id)
		remove_timeout ();
	timeout_id = add_timeout (magic_number, ...);

	? simpler flow, more lucid, no cut and paste.

> +		/* Check if a window with this uri is already open. If there
> +		 * is, then position the new window at a slightly different
> +		 * location so that it doesn't fully cover the existing window.
> +		 */

	Ok - this is the bit I can really disagree with :-) There is quite a
simple solution I think - which is to ensure that we don't set the
position (wrong) to start with.

	ie. before we call eel_gtk_window_set_initial_geometry in
nautilus-window-manage-views.c (position_and_show_window_callback): we
scan the list, and if a duplicate is showing the current window [ surely
we must have checked for this already in there ], then we either:

	* pass a new flag 'ignore_position' to
	  eel_gtk_window_set_initial_geometry
or
	* clobber geometry_flags &= ~(EEL_GDK_X_VALUE | 
				      EEL_GDK_Y_VALUE)

	So that the position isn't set - and so the WM can do it's stuff
without interference [ hopefully ].

	How does that sound ? Thanks for looking at it, we're getting there,

	Regards,

		Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot