Restore window position and size does not work after startup

Bug #1642034 reported by Francis Chin on 2016-11-15
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
psensor
Undecided
Jean-Philippe Orsini

Bug Description

Even when preferences are set to "Restore window position and size" this does not take effect apart from once at startup. This bug exists with the version in the Ubuntu 16.04 repository (1.1.3), 1.1.5, and also master (1.2.0).

I have checked how the Compiz "Place Windows" plugin affects the positioning of windows on the desktop, and this is deciding where the psensor main window opens even with the psensor configured to control the window position. With the latter enabled, the user expects the window to reopen at the same location on the desktop, but this isn't happening.

I took a look at the source code and watched dconf-editor to try and understand what was going on. With debug logging on, I could see that when the UI window is opened correctly, gtk_window_move() in ui_window_create() does the right thing. Yet when the window is closed and then reopened, it's handled by ui_window_show() and the window is placed by Compiz's plugin rather than reappearing at its last location.

I attach a patch to call gtk_window_move() in ui_window_show() and this fixes the problem for me; I see the window reopening at the location persisted in Gsettings. Further testing with "Restore window position and size" cleared (and psensor restarted) shows that window size and position is determined by the WM, as expected.

I also saw that save_window_pos() is already called in ui_psensor_quit() so updated on_delete_event_cb() to move it inside the if statement.

The patch passed "make clean", and was tested applied to my clone of master (1.2.0).

Francis Chin (chinf) on 2016-11-15
description: updated
Revision history for this message
Francis Chin (chinf) wrote :

Updated patch to remove unnecessary diff of po/Makefile.in

Revision history for this message
Francis Chin (chinf) wrote :

I attach a new patch for a few related issues:

- 30s after opening, the pane divider on the main window would adjust itself back to an old position. Also, setting preferences would often revert the pane position in a similar way. I tracked the former bug down to initial_window_show() calling ui_window_update() for a check which I understand is to ensure that psensor can't hide its main window if there is no appindicator/status icon - as the user would then have no GUI to interact with. I have updated the patch so initial_window_show (which I think should be named check_ui_visible() or similar) does not call ui_window_update() as there is no need. ui_pref_dialog_run() also had a similar redundant call. Given there were then no functions calling ui_window_update() other than ui_window_show(), I merged the two.

- The preference "Restore position and size" was under the Startup tab, but I don't believe the intent was to only have the window position retained for just startup and not afterwards; if a user wants it in a particular place (and not have the WM decide where it goes), that's unlikely to be just for one occasion. Hence I updated the patch to move the preference to the Interface tab and renamed it "Keep window position and size". Unrelated: I also suggest "Data sources" might be a better name for the "Providers" tab.

The translations would need to be updated if this patch is accepted. When I cloned from git master, I got po/Makefile.in.in and po/Makefile.in; building psensor results in the latter being deleted. Other than this, I haven't made any changes to the po directory.

Revision history for this message
Francis Chin (chinf) wrote :

Note the last patch supersedes the prior one.

Revision history for this message
Francis Chin (chinf) wrote :

I see that using the "Show" command from the app indicator menu will call ui_window_show() even if the window is already open, so the first problem in my comment #3 applies to this command too. Not sure yet what the best way to fix this would be.

Revision history for this message
Francis Chin (chinf) wrote :

Updated patch with fix for Show command reverting pane sizing.

I've also noticed that the main window size is in fact always restored (the code does not take account of any preference setting for this), so I propose renaming the "Keep/Restore window position and size" preference to just "Keep window position".

Revision history for this message
Jean-Philippe Orsini (jfi) wrote :

Hello Francis,

One more time, thanks for your contribution, I am going to work on it.

There are different issues, I am going to split this one.

Changed in psensor:
assignee: nobody → Jean-Philippe Orsini (jfi)
status: New → In Progress
Revision history for this message
Jean-Philippe Orsini (jfi) wrote :
Revision history for this message
Jean-Philippe Orsini (jfi) wrote :
Revision history for this message
Jean-Philippe Orsini (jfi) wrote :

- if (!ui->config->hide_on_startup
- || (!is_appindicator_supported() && !is_status_supported()))
+ if (!is_appindicator_supported() && !is_status_supported())
   ui_window_show(ui);

If AppIndicator is not support and GTK status icon is not supported, we have to show the window otherwise the user will not have any way to show it (except running again psensor...) .

If AppIndicator or/and GKT Status icon is supported we have to respect the setting "hide on startup".

Revision history for this message
Jean-Philippe Orsini (jfi) wrote :

About "Provider" / "Data sources", I dont have a strong opinion as I am really not a native english speaker, so I trust you.

The UI change (move to interface tab) makes sense, I agree.

except #10, the remaining parts of #3 sounds very good to me, I will split into several patches asap.

Thanks again Francis, sorry to be slow to merge all your patches, but be sure that I will read all the patches you did and appreciated a lot your work.

Revision history for this message
Jean-Philippe Orsini (jfi) wrote :

My comment #10 is wrong, your change is correct because initial_window_show is only called in your patch when "hide_on_startup" is true, I miss the impact of the other change. So, !ui->config->hide_on_startup is always false and can be removed.

Revision history for this message
Jean-Philippe Orsini (jfi) wrote :
Revision history for this message
Francis Chin (chinf) wrote :

Sorry about #10, I ought to have made it clearer to you about how I'd already taken into account the need to keep a UI showing in that scenario.

Thank you for finding the time to look into this.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers