psensor

Restore window position and size does not work after startup

Bug #1642034 reported by Francis Chin on 2016-11-15

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	psensor	In Progress	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).

See original description

Francis Chin (chinf) on 2016-11-15

description:

updated

Revision history for this message

Francis Chin (chinf) wrote on 2016-11-15:

Patch to fix "restore window position" Edit (959 bytes, text/plain)

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

Revision history for this message

Francis Chin (chinf) wrote on 2016-11-24:

fix pane resizing and update to preferences dialog Edit (22.2 KiB, text/plain)

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 on 2016-11-24:

Note the last patch supersedes the prior one.

Revision history for this message

Francis Chin (chinf) wrote on 2016-11-25:

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 on 2016-11-29:

add pane resize fix for Show command Edit (37.1 KiB, text/plain)

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 on 2017-02-12:

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 on 2017-02-16:

Merged patch to avoid useless saving of the window position
http://git.wpitchoune.net/gitweb/?p=psensor.git;a=commit;h=3e708ba44ecfc86f33fc06b6060b77740c1b50c9

Revision history for this message

Jean-Philippe Orsini (jfi) wrote on 2017-02-16:

Merged patch to always move at the save position the window:
http://git.wpitchoune.net/gitweb/?p=psensor.git;a=commit;h=90ec04c6efb1f0b9e3887a86b1da9e7d0d33f16f

Revision history for this message

Jean-Philippe Orsini (jfi) wrote on 2017-02-16:

#10

- 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 on 2017-02-16:

#11

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 on 2017-02-17:

#12

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 on 2017-02-17:

#13

http://git.wpitchoune.net/gitweb/?p=psensor.git;a=commit;h=7264bd6090d2daa896959ed7ca86ea63365b9a90
http://git.wpitchoune.net/gitweb/?p=psensor.git;a=commit;h=e6444eb3fa27922e70f4ae2bbcd32a3d6f295ab8
http://git.wpitchoune.net/gitweb/?p=psensor.git;a=commit;h=c1e20f2631a1249720e9c75d753eacfcb0f6c7b9