GNOME Bugzilla – Bug 702507
Better default positioning of UI components / panes
Last modified: 2014-09-28 16:49:40 UTC
In pitivi's mainwindow.py, at the end of the createUi method, we have code trying to auto-guess sane defaults to position the various hpanes/vpanes. Except that it's a bit crap. There ought to be a better "magic" calculation for it. Maybe the only way is to assume that it should be divided like this: --------------------------------- | || || | | <-33%-> || <-33%-> || <-33%-> | | || || | | || || | ================================= | ^^^ | | 50% | | vvv | | | --------------------------------- Not sure how well it scales for the following screen resolutions: - 1024x768 - 1280x720 - 1920x1080 - "retina" displays of the future
Hello Jean-François, I'm a new adventurer trying to get started with GNOME development, specially Pitivi, and I chose this bug to start. Since it's a trivial bug, I think it's a good bug to fix.
Created attachment 271129 [details] [review] Default placement patch Changes the default divisions of the main window's widgets, follows the standard: - Horizontal widgets fills 1/3 of the screen each one. - Vertical widgets fills half of the screen each one.
Created attachment 271132 [details] [review] Better default positioning patch Fixed a small mistake. Now waits for the window to setup it's size, and then calculates the default positioning as described.
Created attachment 271283 [details] screenshot of the result of patch v2 Hi Georges Basile (am I guessing the first name correctly?), I gave your 2nd patch a try on my 1920x1080 desktop but it does not seem to calculate things correctly, as the attached screenshot demonstrates... do you have multiple resolutions to test with? Also, you may be curious about the existence of bug #723061 too (haven't found out what causes that one), though it's not directly related.
Guessed correctly, and thanks for the quick reply! I was quite sure of the patch, but now I'm thinking that my jhbuild setup is broken. I'm having a hard time trying to enable the gnome-world-3.12 moduleset. I'll check that later. Anyway, maybe these bugs are related to the deprecation of Gtk.HPaned and Gtk.VPaned. I'll correct this too, it's fairly simple.
Created attachment 271291 [details] [review] Better default layout patch This new version was moderately tested. I'm pretty sure it works as expected now. Also, removes the use of Gtk.VPaned and Gtk.HPaned (both deprecated) to Gtk.Paned.
Your latest patch is a good step in the right direction, it "mostly works" on my end, however the fact that it doesn't put the position of the vpane exactly in the middle of my screen/window made me suspect there's a small mistake somewhere. If I print the "window_size" variable you added, - On a fresh startup (no pitivi.conf file), I get: (1920, 712) - On subsequent startups, I will get (everytime): (1920, 1015) Given that 712 px height (vs my 1080px screen height), I can suspect that the height calculations will be wrong initially, which is exactly when we're trying to set the default values for the pane positions :)... Also, beyond that issue, I'm wondering if you need to take window decorations & main toolbar* into account to calculate only the height of the "available area for the vpane" to set the relative position (not 100% sure about my hypothesis here, just thinking out loud :) *: that one eventually becomes irrelevant with the headerbar branch
I'm sorry, on my hardware it works flawlessly. Currently, it's not taking window decorations and the main toolbar into account. The toolbar itself is not a problem, but I think it's actually impossible to do it with the window decoration. I'll work on the (hopefully) last patch. Maybe on the next bugs I can do better!
May I keep the replacement of Gtk.VPaned & Gtk.HPaned in the patch? Since both are deprecated in favor of Gtk.Paned, it's a good idea to move to it.
> Maybe on the next bugs I can do better! No stress. Nobody expects perfection! An iterative approach is fine, and learning in the process is more valuable than getting it right "magically" the very first time (which is quite unlikely anyway ;) > May I keep the replacement of Gtk.VPaned & Gtk.HPaned in the patch? > both are deprecated in favor of Gtk.Paned Yes, absolutely. We sometimes have pieces of code like this that have not been ported to newer APIs; doing this cleanup as part of other improvements that touch the same lines is perfectly acceptable. Thanks!
Created attachment 271571 [details] [review] Better default placement for widgets v4 This time I used the "configure-event" event to calculate the vpanel default placement. I still think there is something wrong, but I can't figure out what it is.
I would suggest a clearer name for "self.setup_placement", maybe: self._positioning_settings_exist And then you could rework/cleanup the lines above, so that you don't have a ton of "if" statements after the "# Restore settings (or set defaults) for position and visibility" So basically: - Check if the width and height of the window are -1 - If yes, then self._positioning_settings_exist = False - Reuse that variable for everything like the stuff above? Sure, it's less precise than checking each and every setting individually, but I'm thinking "bah, why do we care about such precision anyway?" Also, I here's a this little code simplification trick, instead of if self.setup_placement: if not self.is_maximized(): return foo ...you could do if maximized and self._positioning_settings_exist: foo But then I also encountered this traceback:
+ Trace 233338
if not self.is_maximized():
...which made me wonder why you're checking for the maximization state to begin with? Taking out the maximization check makes the thing work without problems on my end.
Created attachment 271944 [details] screenshots with the v4 patch, wide and portrait mode And here's a corner case: if you flip your screen 90 degrees into portrait mode, the 50% height position algorithm fails to do what users would expect... so, for this particular case, I could suggest "50% unless it exceeds a certain height in pixels (ex: 600px for "normal"* screens) *: the problem with my idea is... hiDPI/retina displays. I suspect you can't really rely on an arbitrary/hardcoded amount of pixels to decide "this is too tall!" :)
Maybe we can use the screen ratio to define the behaviour. For example: - 1024x768, ratio = 4:3 (1,3334) - 1366x768, ratio = 16:9 (1,7778) - 1920x1080, ratio = 16:9 (1,7778) if the screen flips 90º, the we have: - 768x1024, ratio = 4:3 (0,75) - 768x1366, ratio = 9:16 (0,5625) - 1080x1920, ratio = 9:16 (0,5625) Since the 4:3 ratio is still reasonably usable, we can mark that as the limit. So, if screen_ratio >= 0,75 do the actual stuff, else... what should be a good default behavior? I think a good default for screen_ratio < 0,75 is to divide vpaned by 3. What do you think, Jean-François? (By the way, we share the same name on different languages! haha random stuff)
Created attachment 272196 [details] [review] Better default placement for widgets v5 Updated to better variable names. Also, this patch calculates screen ratio and adapts VPaned position accordingly. I'm not sure I understood the "reuse that variable" comment.
Created attachment 272719 [details] [review] Better default placement for widgets v7 Actually I found out why the VPaned had a default value, while MainHPaned and SecondHPaned didn't. It is fixed now, using the default behaviour defined some comments ago (1/2 for normal screens, 1/3 for tall screens).
Review of attachment 272719 [details] [review]: Note: for testing, I'm running "rm ~/.config/pitivi/pitivi.conf ; pitivi" Here's a review/test of your latest patch: > # mainhpane separator placed at 2/3 of the screen. > self.mainhpaned.set_position(2 * self.get_size()[0] / 3) > # secondhpane is 2/3 of the screen width, 1/3 needed > self.secondhpaned.set_position(self.get_size()[0] / 3) Maybe use a local variable right before this stuff, "window_width" instead of the somewhat unclear "self.get_size()[0]" being called twice. > window_width = float(self.get_screen().get_width()) > window_height = float(self.get_screen().get_height()) Those variable names are misleading... screen_width and screen_height would be the ones to use instead. > if window_width / window_height < 0.75: > self.vpaned.set_position(self.vpaned.get_allocation().height / 3) > else: > self.vpaned.set_position(self.vpaned.get_allocation().height / 2) According to my testing (for some reason, in widescreen mode, the divider between the timeline and the rest was not in the middle of my screen's height), this will always fail because self.vpaned.get_allocation().height immediately returns the value "1" at startup... Consider using self.vpaned.size_request().height (this makes it return a correct value on my end; I borrowed that from utils/widgets.py, however, that *may* not be the correct future-proof way as https://developer.gnome.org/gtk3/stable/ch24s02.html indicates... Furthermore, the set_position call always fails, it seems. Even if I hardcode a value such as set_position(100), its placement seems to always be somewhere else on my screen. I'm a bit puzzled about that, I'm not sure why I run into problems you're probably not seeing on your end - maybe my computer (core2 quad with a solid state drive) triggers race conditions more easily? :/ Your aspect-ratio-based heuristic sounds like a good idea though!
Review of attachment 272719 [details] [review]: In the block below "# Restore settings (or set defaults) for position and visibility" just above your changes you can see we call for example "self.mainhpaned.set_position" - I think the code would be much clearer if you make a method getPositions which returns a tuple and you use the returned values in the set_position calls. Hope it makes sense.
Created attachment 273313 [details] [review] Better default placement for widgets v8 Implemented as Alex said. The code is so much cleaner now. Again, it works flawlessly on my hardware, but it needs more testing though. Also, why doesn't Pitivi save the maximized state of the window?
Any review or comment on the patch? It's been a while since last response.
Created attachment 276273 [details] [review] Fix by making sure the settings have default values Sorry, I thought I replied. Here is an even more simple patch I'm proposing, based on yours. It always uses the settings and before that makes sure there are at least some default values - see _setDefaultPositions and how it's used.
Very nice modifications, Alex! I learned a lot with this trivial bug :)
Review of attachment 276273 [details] [review]: OK
Review of attachment 276273 [details] [review]: Nice code implementation, however it's still buggy/not working as expected :( The positioning of the pane separating the timeline from the rest is still wrong on my 1920x1080 screen, both in landscape and portrait (1080x1920) mode. At first I thought it was due to the use of "self.vpaned.size_request().height" to calculate the target vpane position... but I swapped it for screen_height and that didn't solve the problem. Then I tested some more and realized that no matter what you do with self.vpaned.set_position, even if you hardcode it to set_position(100) or set_position(1000), it totally ignores you. What the hell... FWIW, this is how I experimented locally: diff --git a/pitivi/mainwindow.py b/pitivi/mainwindow.py index d013cb4..53ab920 100644 --- a/pitivi/mainwindow.py +++ b/pitivi/mainwindow.py @@ -311,7 +311,9 @@ class PitiviMainWindow(Gtk.ApplicationWindow, Loggable): self.move(self.settings.mainWindowX, self.settings.mainWindowY) self.secondhpaned.set_position(self.settings.mainWindowHPanePosition) self.mainhpaned.set_position(self.settings.mainWindowMainHPanePosition) + print("current vpane position:", self.vpaned.get_position()) self.vpaned.set_position(self.settings.mainWindowVPanePosition) + print("vpane position set to:", self.vpaned.get_position()) # Connect the main window's signals at the end, to avoid messing around # with the restoration of settings above. @@ -331,11 +333,17 @@ class PitiviMainWindow(Gtk.ApplicationWindow, Loggable): if self.settings.mainWindowVPanePosition is None: screen_width = float(self.get_screen().get_width()) screen_height = float(self.get_screen().get_height()) + print("screen dimensions are", screen_width, screen_height) if screen_width / screen_height < 0.75: # Tall screen, give some more vertical space the the tabs. - value = self.vpaned.size_request().height / 3 + #value = self.vpaned.size_request().height / 3 + value = screen_height / 3 + print("tall screen!") else: - value = self.vpaned.size_request().height / 2 + #value = self.vpaned.size_request().height / 2 + value = screen_height / 2 + print("not a tall screen") + print("vpaned size is %s, screen height is %s, final value is %s" % (self.vpaned.size_request( self.settings.mainWindowVPanePosition = value def switchContextTab(self, bElement):
Created attachment 278090 [details] screenshot with aleb's patch (v9)
So there is more work needed for that?
I think so, as I explained in comment #24 it still doesn't work right for me and so ideally I'd like it to work "perfectly" to be sure the patch's behavior is correct too...
It's a vpaned bug, I filed https://bugzilla.gnome.org/show_bug.cgi?id=733638
Fixed for the upcoming release (with Aleb's patch as commit 374aa642ada7), the rest is bug #733638 in GTK I suppose...