Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wxGUI: Default Single-Window size is too small #3024

Conversation

lindakarlovska
Copy link
Contributor

Fixes #3013

@lindakarlovska lindakarlovska self-assigned this Jun 5, 2023
@lindakarlovska lindakarlovska added GUI wxGUI related bug Something isn't working labels Jun 5, 2023
@lindakarlovska lindakarlovska added this to the 8.3.0 milestone Jun 5, 2023
@petrasovaa petrasovaa requested a review from pesekon2 June 5, 2023 14:28
@wenzeslaus wenzeslaus modified the milestones: 8.3.0, 8.3.1 Jun 6, 2023
@pesekon2
Copy link
Contributor

pesekon2 commented Jun 6, 2023

Fixes the problem for me. @tmszi does it fix also your TypeError from #3013?

@lindakarlovska
Copy link
Contributor Author

Fixes the problem for me. @tmszi does it fix also your TypeError from #3013?

@pesekon2 Most likely, it is not going to help. The MenuItem error has nothing to do with the size of Single Window. I would suggest creating a different PR for it.

@lindakarlovska lindakarlovska marked this pull request as draft June 6, 2023 13:26
@petrasovaa
Copy link
Contributor

Should this go to 8.3 release? It looks like a an important fix. Why is it draft? The TypeError is unrelated.

@lindakarlovska
Copy link
Contributor Author

Should this go to 8.3 release? It looks like a an important fix. Why is it draft? The TypeError is unrelated.

Yes, it should go to the release 8.3.0, I initially set it up for version 8.3.0.

This is a draft because I would like to test it more (it was very fast written).

@landam landam added the backport to 8.3 PR needs to be backported to release branch 8.3 label Jun 7, 2023
@landam landam modified the milestones: 8.3.1, 8.4.0 Jun 7, 2023
@landam
Copy link
Member

landam commented Jun 7, 2023

Single Window should probably start maximized by default. @lindakladivova @petrasovaa Any opinion?

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jun 7, 2023

Single Window should probably start maximized by default. @lindakladivova @petrasovaa Any opinion?

Well @landa, it actually is maximized by default but the problem is that we take the parameter defWindowPos (set by globalvar.GM_WINDOW_SIZE variable) from UserSettings if defined (rows 181 - 196). You can go to Settings -> General -> and check Suppress positioning Layer Manager window, close GRASS and start again and it will launch as maximized.

IMHO This User setting does not make sense for Single-Window, I would suggest removing rows 181 - 196 in frame/main_window.py. @petrasovaa what do you think?

Reminding me another thing (not related to this PR but also important): after the successful merge of #2667 we could finally remove the lmgr/frame.py file and possibly some other stuff like globalvar.GM_WINDOW_SIZE. Any opinions on this?

Further, I have one more thing here, I would like to try to suppress the GTK warnings as @vero mentioned... it is also painful but I will at least try it and we will see..

@tmszi
Copy link
Member

tmszi commented Jun 7, 2023

Well @landa, it actually is maximized by default but the problem is that we take the parameter defWindowPos (set by globalvar.GM_WINDOW_SIZE variable) from UserSettings if defined (rows 181 - 196). You can go to Settings -> General -> and check Suppress positioning Layer Manager window, close GRASS and start again and it will launch as maximized.

By default without using this PR, single window mode size depend on the DE, respectively the type of window layout (floating, tiled, monocle) and OS wxGTK, wxMSW.

wxMSW (floating windows layout) default single window size and position:

wxmsw_swm_size_pos_floating

wxGTK (floating windows layout with dwm window manager) default single window size and position (issue #3013):

wxgtk_smw_size_pos_dwm_floating

wxGTK (tiled/monocle windows layout with dwm window manager) default single window size and position:

wxgtk_swm_dwm_def_pos_size_tiled

wxGTK (floating windows layout with Gnome DE) default single window size and position:

wxgtk_smw_size_pos_gnome_floating

Important user settings is Save current window layout as default which works as expected. You can change position and size of window (single window mode too).

IMHO This User setting does not make sense for Single-Window, I would suggest removing rows 181 - 196 in frame/main_window.py. @petrasovaa what do you think?

It is not a good idea, if you remove these lines, the wxGUI single window mode will not respect the client area and will be overlapped by the DE panel(s).

@pesekon2
Copy link
Contributor

pesekon2 commented Jun 8, 2023

Further, I have one more thing here, I would like to try to suppress the GTK warnings as @vero mentioned... it is also painful but I will at least try it and we will see..

This would be the best thing to happen in GRASS for many years, in my eyes (I have the experience that when the users see those warnings and criticals, they consider GRASS to be broken and stop using it). Have had a discussion about that with @wenzeslaus and he can give a verbose, loquacious monologue on that. But we shouldn't make this issue a discussion forum keep sticking to the topic.

@tmszi
Copy link
Member

tmszi commented Jun 8, 2023

Seems it is regression between revisions (respectively GRASS GIS versions)

  1. Single window mode launch in the full screen mode by default with GRASS GIS version 8.3.0dev

wxGTK (floating windows layout with Gnome DE) default single window size and position:

wxgtk_swm_def_pos_size_floating

GRASS world_latlong_wgs84/PERMANENT:~ > g.version -g
version=8.3.0dev
date=2023
revision=567ea066bd
build_date=2023-06-07
  1. Single window mode doesn't launch in the full screen mode by default with GRASS GIS version 8.4.0dev

wxGTK (floating windows layout with Gnome DE) default single window size and position:

wxgtk_swm_def_pos_size_floating

GRASS world_latlong_wgs84/PERMANENT:~ > g.version -g
version=8.4.0dev
date=2023
revision=24fd2a38f4
build_date=2023-06-08

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jun 9, 2023

@tmszi I will try to explain how I understand the problem, I might be wrong, please correct me.

The size of wx.Frame is in the constructor given by wx.Display().GetGeometry().GetSize() by default (in my case e.g. (1920, 1080)).

But the problem is that after calling self.Fit(), for some reason, the size of the main window is being changed and this is why I removed all self.Fit() callings. Not sure what happened between 8.3.0dev and 8.4.0dev versions but suddenly self.Fit() did not work.

But! When I tested this Pull request I was quite annoyed by the fact that even after removing the Fit() calling, I could not reach the maximized extent (1920, 1080). The problem was that I was actually still overwriting the desired extent by the extent I had saved in the UserSettings (rows 181-197).

Maybe I am not right, but I think that using just wx.Display().GetGeometry().GetSize() could be enough for setting the size of the main window. (I went from the fact that also other software I use always starts over the whole screen at the beginning), I think that making Single-Window smaller and saving the adapted extent into the User Settings does not make much sense...

But we can let it as it is, I have no problem with that. I was just critically thinking about the code and if these 17 rows have some meaning..

@lindakarlovska
Copy link
Contributor Author

lindakarlovska commented Jun 9, 2023

Further, I have one more thing here, I would like to try to suppress the GTK warnings as @vero mentioned... it is also painful but I will at least try it and we will see..

This would be the best thing to happen in GRASS for many years, in my eyes (I have the experience that when the users see those warnings and criticals, they consider GRASS to be broken and stop using it). Have had a discussion about that with @wenzeslaus and he can give a verbose, loquacious monologue on that. But we shouldn't make this issue a discussion forum keep sticking to the topic.

@pesekon2 I have spent two hours thinking about how to suppress these GTK warnings when launching GRASS and no results. I just succeeded in making more of them :)...

@lindakarlovska lindakarlovska marked this pull request as ready for review June 9, 2023 15:20
@wenzeslaus
Copy link
Member

...I would like to try to suppress the GTK warnings as @vero mentioned... it is also painful but I will at least try it and we will see..

This would be the best thing to happen in GRASS for many years, in my eyes (I have the experience that when the users see those warnings and criticals, they consider GRASS to be broken and stop using it). Have had a discussion about that with @wenzeslaus and he can give a verbose, loquacious monologue on that...

...I have spent two hours thinking about how to suppress these GTK warnings when launching GRASS and no results...

#3049 removes the stderr when GUI is started together with GRASS GIS (not for g.gui).

@tmszi
Copy link
Member

tmszi commented Jun 10, 2023

@tmszi I will try to explain how I understand the problem, I might be wrong, please correct me.

The size of wx.Frame is in the constructor given by wx.Display().GetGeometry().GetSize() by default (in my case e.g. (1920, 1080)).

But the problem is that after calling self.Fit(), for some reason, the size of the main window is being changed and this is why I removed all self.Fit() callings. Not sure what happened between 8.3.0dev and 8.4.0dev versions but suddenly self.Fit() did not work.

But! When I tested this Pull request I was quite annoyed by the fact that even after removing the Fit() calling, I could not reach the maximized extent (1920, 1080). The problem was that I was actually still overwriting the desired extent by the extent I had saved in the UserSettings (rows 181-197).

Maybe I am not right, but I think that using just wx.Display().GetGeometry().GetSize() could be enough for setting the size of the main window. (I went from the fact that also other software I use always starts over the whole screen at the beginning), I think that making Single-Window smaller and saving the adapted extent into the User Settings does not make much sense...

But we can let it as it is, I have no problem with that. I was just critically thinking about the code and if these 17 rows have some meaning..

@lindakladivova First thanks for your explanation.

I attached my patch which solve two issues:

  1. Maximize single window mode by default
  2. Respect user custom window position and size (custom win pos and size for single win mode is saved as standalone settings separate from multi win mode settings) changed via wxGUI Preference dialog Save current window layout as default checkbox widget

git am 0001-wxGUI-main_window-maximize-window-by-default.patch

0001-wxGUI-main_window-maximize-window-by-default.patch.zip

@tmszi
Copy link
Member

tmszi commented Jun 18, 2023

Tested on Gentoo with dwm with activated floating window mode and wxGUI has been launched maximized by default and custom user settings (win position and size) works as expected too.

Copy link
Contributor

@petrasovaa petrasovaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with multi-window and currently it ignores the map display size and position, because IsDocked is True for mutli-window. This should fix it:

--- a/gui/wxpython/gui_core/preferences.py
+++ b/gui/wxpython/gui_core/preferences.py
@@ -1853,7 +1853,9 @@ class PreferencesDialog(PreferencesBaseDialog):
 
                 # window size must be larger than zero, not minimized
                 # do not save dim when mapdisp is docked within single window
-                if not mapdisp.IsDocked() and (size[0] > 0 and size[1] > 0):
+                if (not mapdisp.IsDockable() or not mapdisp.IsDocked()) and (
+                    size[0] > 0 and size[1] > 0
+                ):
                     dim += f",{pos[0]},{pos[1]},{size[0]},{size[1]}"
 
             if single_window:

@landam
Copy link
Member

landam commented Jun 20, 2023

I tested multi-window with the patch above applied.

  1. started GUI in multi-window layout
  2. changed windows position
  3. go to preferences, save
  4. restart GUI

Windows appeared in original position, new position was ignored.

It seems that there is still an issue.

landam
landam previously requested changes Jun 20, 2023
Copy link
Member

@landam landam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multi-window position is ignored

@landam
Copy link
Member

landam commented Jun 20, 2023

I have

"dim": "26,23,634,622,26,23,827,676",

in settings regardless how I organize windows.

@landam
Copy link
Member

landam commented Jun 20, 2023

Strangely, when I change size of windows it works. But position is always the same:

26,23,866,802,26,23,951,762

@landam
Copy link
Member

landam commented Jun 20, 2023

GetPosition() always returns (26,23) in my case (wxpython 4.2.0). But it seems to be not related to this PR.

@lindakarlovska lindakarlovska force-pushed the wxGUI-default-single-layout-size-too-small branch from a67fe98 to bc9f85b Compare June 20, 2023 16:45
@lindakarlovska lindakarlovska force-pushed the wxGUI-default-single-layout-size-too-small branch from bc9f85b to f505365 Compare June 20, 2023 16:53
@lindakarlovska
Copy link
Contributor Author

GetPosition() always returns (26,23) in my case (wxpython 4.2.0). But it seems to be not related to this PR.

Yep, that's unrelated (probably a wxPython 4.2.0 error).

@petrasovaa
Copy link
Contributor

GetPosition() always returns (26,23) in my case (wxpython 4.2.0). But it seems to be not related to this PR.

Yep, that's unrelated (probably a wxPython 4.2.0 error).

For me the position is saved as expected, I also use wxPython 4.2.0, so I am unsure why it doesn't work.

@petrasovaa petrasovaa modified the milestones: 8.4.0, 8.3.0 Jun 21, 2023
@petrasovaa petrasovaa merged commit 8a9e840 into OSGeo:main Jun 21, 2023
19 checks passed
petrasovaa pushed a commit that referenced this pull request Jun 21, 2023
Maximize or set dimensions from user settings.
@petrasovaa petrasovaa removed the backport to 8.3 PR needs to be backported to release branch 8.3 label Jun 21, 2023
@petrasovaa
Copy link
Contributor

In the interest of timely release I took liberty to merge and backport to 8_3 branch.

landam pushed a commit to landam/grass that referenced this pull request Oct 25, 2023
Maximize or set dimensions from user settings.
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
Maximize or set dimensions from user settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working GUI wxGUI related
Development

Successfully merging this pull request may close these issues.

[Bug] Default single-window size is too small to see anything
7 participants