GNOME Bugzilla – Bug 664779
Add height for width layout mode in horizontal orientation.
Last modified: 2018-01-24 13:50:41 UTC
I'm working on a pager panel plugin (for XFCE) using a WNCK pager. The pager is instantiated horizontally even though the panel itself is in a vertical mode. Unfortunately WNCK pager, when in horizontal mode, defaults to a width for height policy, which interferes badly with the vertical panel (it forces the width and expects the plugins to decide the height on their own). Currently I have a workaround for this behavior in my plugin code but it is nasty hack (in some situations it may result in infinite event looping). I'd really like this issue to be addressed in the WNCK pager itself, which would be an easy fix. For your reference, the plugin using the WNCK pager is in: https://github.com/andrzej-r/xfce4-panel/blob/deskbar_mode/plugins/pager/pager.c The relevant commits working around the WNCK pager limitations are: https://github.com/andrzej-r/xfce4-panel/commit/a13e5d342d24c20b305a675af99e419eaecc5a0c https://github.com/andrzej-r/xfce4-panel/commit/e374978220d11adc1830b383d557817aca5a0a32 https://github.com/andrzej-r/xfce4-panel/commit/66a2044633a33942e746d4e1a8c55653863e1c00 https://github.com/andrzej-r/xfce4-panel/commit/0d785dc80ee6c00e0fab76d422137df0def18eb4 I imagine adding such functionality would require extending the API with something like: void wnck_pager_set_layout_policy (WnckPager *pager, WnckPagerLayoutPolicy policy); typedef enum { WNCK_PAGER_LAYOUT_POLICY_AUTOMATIC, //current behavior, default WNCK_PAGER_LAYOUT_POLICY_WIDTH_FOR_HEIGHT, WNCK_PAGER_LAYOUT_POLICY_HEIGHT_FOR_WIDTH } WnckPagerLayoutPolicy; If such API extension can be accepted, I'll go ahead and implement this feature on top of libwnck-2.30.
I guess that means we could also re-use most of the code for libwnck 3, right? Feel free to go ahead.
Great! BTW, do you have any timeline for releasing a new 2.30.x (or 2.32?) version? I've mentioned version 2.30 only because XFCE pager plugin currently depends on it and that's what I'm using for testing. Nevertheless, once it's done the code shouldn't be hard to port it to 3.x. I'll look into it later.
(In reply to comment #2) > Great! BTW, do you have any timeline for releasing a new 2.30.x (or 2.32?) > version? Nope. I can do a tarball whenever we need it.
Created attachment 202149 [details] [review] A patch for forced layout policy (bug 664779) A patch implementing this feature on top of libwnck-2.30.(8). Tested 4 cases (2 orientations x 2 forced policy values) with display_mode==WNCK_PAGER_DISPLAY_CONTENT.
Hrm, something feels wrong in the patch, but I don't think it's because of the patch itself. It's probably just this long-standing confusion about "orientation of the layout of all workspaces" and "orientation of the pager widget". Thinking about this, I think I'd like to fix this this way: - (optional, possibly only needed for me to understand what I'm writing ;-)) keep your API, but internally, translate it to a "widget orientation" - deprecate wnck_pager_set_orientation(); would be replaced by what's below - add wnck_pager_set_layout_orientation(), that would have three potential values (AUTO, HORIZONTAL, VERTICAL). AUTO would be used to automatically choose set the layout orientation to HORIZONTAL or VERTICAL, based on the internal widget orientation (ie, that'd keep compatibility with what we have today). HORIZONTAL/VERTICAL could be used otherwise to force a specific layout. This is all assuming we can own the workspaces layout; if we don't own it, then we'd just respect what is set. And then, this would all make a bit more sense, I think. Or am I confused? :-)
Vincent, I think this particular issue is orthogonal to our discussion on decoupling pager and workspace layouts. This is quite a bit simpler - the pager assumes that the *container* orientation (panel in our case) is same as the orientation of the pager itself. Because of that, if a horizontal pager is embedded in a vertical panel, it incorrectly calculates its size requisition (assumes that height is "given" and width is "variable", when the opposite is true). The result is that the aspect ratio of the pager widget is incorrect. Returning to the other issue - decoupling pager and workspace layouts. Yes, we are in favor of having separate knobs for them. Mainly because we want to put "number of rows" in the desktop-wide workspace settings and not in the settings of one of the plugins in the panel. I understand you prefer the other solution so the only thing we as for is making the "coupling" optional. Note that it is already (almost) optional - if there are more than one pager widgets all but the first one will be "decoupled" from the workspace layout. As for the API for setting the orientation, how about keeping wnck_pager_set_orientation/n_rows() and adding wnck_screen_set_orientation/n_rows() ? If the "coupling" was enabled and the pager owned the workspace layout setting pager orientation would (as it is now) change the workspace layout. If not, the user could do it manually using wnck_screen_set... functions.
(In reply to comment #6) > Vincent, I think this particular issue is orthogonal to our discussion on > decoupling pager and workspace layouts. This is quite a bit simpler - the pager > assumes that the *container* orientation (panel in our case) is same as the > orientation of the pager itself. Because of that, if a horizontal pager is > embedded in a vertical panel, it incorrectly calculates its size requisition > (assumes that height is "given" and width is "variable", when the opposite is > true). The result is that the aspect ratio of the pager widget is incorrect. I do understand what the intent of the patch is. The issue I have is that, with this patch, we're making a bad situation even worse. This is why I feel decoupling the widget layout from the workspace layout would benefit us here. > Returning to the other issue - decoupling pager and workspace layouts. Yes, we > are in favor of having separate knobs for them. Mainly because we want to put > "number of rows" in the desktop-wide workspace settings and not in the settings > of one of the plugins in the panel. I understand you prefer the other solution > so the only thing we as for is making the "coupling" optional. Note that it is > already (almost) optional - if there are more than one pager widgets all but > the first one will be "decoupled" from the workspace layout. Right, the code is mostly ready already. (And it's not that I prefer any solution; it's just that I prefer to keep the default to the current behavior to not break things for anyone). > As for the API for setting the orientation, how about keeping > wnck_pager_set_orientation/n_rows() and adding > wnck_screen_set_orientation/n_rows() ? > If the "coupling" was enabled and the pager owned the workspace layout setting > pager orientation would (as it is now) change the workspace layout. If not, the > user could do it manually using wnck_screen_set... functions. We have wnck_screen_try_set_workspace_layout() already. It's not a good API, though (it doesn't have anything about orientation), and we should provide a better API for this. I think your patch in bug 664780 might even help us move closer to what we'd like. FWIW, a better API would be: int wnck_screen_try_own_workspace_layout(WnckScreen *screen); gboolean wnck_screen_set_workspace_layout(WnckScreen *screen, int token, WnckLayoutOrientation orientation, int rows, int columns, WnckLayoutCorner starting_corner); void wnck_screen_unown_workspace_layout(WnckScreen *screen, int token); (or well, we keep wnck_screen_release_workspace_layout() instead of the last one, it's the same function)
So I'd suggest this plan: a) Review WnckScreen API to get workspace layout and make it public if ready b) Add improved WnckScreen API to set workspace layout c) Port everything in libwnck using the old API to set workspace layout to the new API d) Add WnckPager API to specify that we don't want to own the workspace layout e) Uncouple the WnckPager mess linking widget orientation to workspace layout, and fix the original bug from comment 1 :-) How does that sound?
(In reply to comment #4) > A patch implementing this feature on top of libwnck-2.30.(8). Tested 4 cases (2 > orientations x 2 forced policy values) with > display_mode==WNCK_PAGER_DISPLAY_CONTENT. Just to clarify, when you said you tested "4 cases (2 orientations x 2 forced policy values)", do you mean you tested with the orientation of the panels, or the orientation of the layout? It feels you should test a combination like: "orientation of panel" x "orientation of layout" x policy value (unless the orientation of the panel and the orientation of the layout are always in sync).
AFAIR, I've tested two pager (layout?) orientations, each with two panel orientations and matching layout policies. I'm now trying the new version - I'll let you know if there is any problem.
I've rechecked the pager in following modes: 1. vertical pager layout in vertical panel (height_for_width policy - same as automatic) 2. horizontal pager layout in vertical panel (height_for_width policy - different from automatic) 3. horizontal pager layout in horizontal panel (width_for_height policy - same as automatic). I haven't tested the remaining one (vertical pager layout in horizontal panel (width_for_height policy - different from automatic) because I don't want to mess with our code but I've done just that before submitting the patch. All tests were passed. Also, versioning API works well, I'll assume that pager layout policy api will be available from version 2.31.0 - please let me know if that's incorrect.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libwnck/issues/123.