After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 702507 - Better default positioning of UI components / panes
Better default positioning of UI components / panes
Status: RESOLVED FIXED
Product: pitivi
Classification: Other
Component: User interface
Git
Other Linux
: Normal trivial
: 0.94
Assigned To: Pitivi maintainers
Pitivi maintainers
Depends on:
Blocks:
 
 
Reported: 2013-06-17 19:47 UTC by Jean-François Fortin Tam
Modified: 2014-09-28 16:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Default placement patch (1.77 KB, patch)
2014-03-06 18:28 UTC, Georges Basile Stavracas Neto
none Details | Review
Better default positioning patch (1.56 KB, patch)
2014-03-06 18:52 UTC, Georges Basile Stavracas Neto
none Details | Review
screenshot of the result of patch v2 (91.14 KB, image/png)
2014-03-07 22:53 UTC, Jean-François Fortin Tam
  Details
Better default layout patch (2.38 KB, patch)
2014-03-08 04:09 UTC, Georges Basile Stavracas Neto
none Details | Review
Better default placement for widgets v4 (3.32 KB, patch)
2014-03-12 03:56 UTC, Georges Basile Stavracas Neto
none Details | Review
screenshots with the v4 patch, wide and portrait mode (215.00 KB, application/x-tar)
2014-03-14 19:34 UTC, Jean-François Fortin Tam
  Details
Better default placement for widgets v5 (3.22 KB, patch)
2014-03-17 18:48 UTC, Georges Basile Stavracas Neto
none Details | Review
Better default placement for widgets v7 (3.10 KB, patch)
2014-03-24 02:25 UTC, Georges Basile Stavracas Neto
needs-work Details | Review
Better default placement for widgets v8 (4.04 KB, patch)
2014-03-31 03:36 UTC, Georges Basile Stavracas Neto
none Details | Review
Fix by making sure the settings have default values (3.40 KB, patch)
2014-05-10 08:07 UTC, Alex Băluț
needs-work Details | Review
screenshot with aleb's patch (v9) (69.30 KB, image/png)
2014-06-07 16:34 UTC, Jean-François Fortin Tam
  Details

Description Jean-François Fortin Tam 2013-06-17 19:47:11 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
Comment 1 Georges Basile Stavracas Neto 2014-03-06 15:44:40 UTC
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.
Comment 2 Georges Basile Stavracas Neto 2014-03-06 18:28:17 UTC
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.
Comment 3 Georges Basile Stavracas Neto 2014-03-06 18:52:47 UTC
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.
Comment 4 Jean-François Fortin Tam 2014-03-07 22:53:22 UTC
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.
Comment 5 Georges Basile Stavracas Neto 2014-03-08 03:16:04 UTC
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.
Comment 6 Georges Basile Stavracas Neto 2014-03-08 04:09:46 UTC
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.
Comment 7 Jean-François Fortin Tam 2014-03-11 16:06:04 UTC
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
Comment 8 Georges Basile Stavracas Neto 2014-03-11 20:32:40 UTC
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!
Comment 9 Georges Basile Stavracas Neto 2014-03-11 20:42:29 UTC
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.
Comment 10 Jean-François Fortin Tam 2014-03-11 20:56:42 UTC
> 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!
Comment 11 Georges Basile Stavracas Neto 2014-03-12 03:56:12 UTC
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.
Comment 12 Jean-François Fortin Tam 2014-03-14 19:21:05 UTC
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:
  • File "pitivi/mainwindow.py", line 581 in _configureCb
    if not self.is_maximized():
AttributeError: 'PitiviMainWindow' object has no attribute '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.
Comment 13 Jean-François Fortin Tam 2014-03-14 19:34:32 UTC
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!" :)
Comment 14 Georges Basile Stavracas Neto 2014-03-14 23:07:02 UTC
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)
Comment 15 Georges Basile Stavracas Neto 2014-03-17 18:48:32 UTC
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.
Comment 16 Georges Basile Stavracas Neto 2014-03-24 02:25:39 UTC
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).
Comment 17 Jean-François Fortin Tam 2014-03-30 02:19:38 UTC
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!
Comment 18 Alex Băluț 2014-03-30 13:17:06 UTC
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.
Comment 19 Georges Basile Stavracas Neto 2014-03-31 03:36:01 UTC
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?
Comment 20 Georges Basile Stavracas Neto 2014-05-10 01:48:14 UTC
Any review or comment on the patch?

It's been a while since last response.
Comment 21 Alex Băluț 2014-05-10 08:07:45 UTC
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.
Comment 22 Georges Basile Stavracas Neto 2014-05-11 12:29:28 UTC
Very nice modifications, Alex!
I learned a lot with this trivial bug :)
Comment 23 Thibault Saunier 2014-05-11 12:41:24 UTC
Review of attachment 276273 [details] [review]:

OK
Comment 24 Jean-François Fortin Tam 2014-06-07 16:29:38 UTC
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):
Comment 25 Jean-François Fortin Tam 2014-06-07 16:34:10 UTC
Created attachment 278090 [details]
screenshot with aleb's patch (v9)
Comment 26 Thibault Saunier 2014-06-20 17:38:11 UTC
So there is more work needed for that?
Comment 27 Jean-François Fortin Tam 2014-06-22 21:36:11 UTC
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...
Comment 28 Alex Băluț 2014-07-23 23:26:13 UTC
It's a vpaned bug, I filed https://bugzilla.gnome.org/show_bug.cgi?id=733638
Comment 29 Jean-François Fortin Tam 2014-09-28 16:49:40 UTC
Fixed for the upcoming release (with Aleb's patch as commit 374aa642ada7), the rest is bug #733638 in GTK I suppose...