https://launchpad.net/distros/ubuntu/+source/libwnck/+bug/63208 is about the same patches and has some explanatory remarks by the authors:


"""
The above patch fixes the above checkbox to work with compiz... from within the patch itself:
"There's no _NET_WM_ACTION_ABOVE hint so always add it if window manager claims to support_NET_WM_STATE_ABOVE"

The viewport patch gives libwnck support for viewports which are used by compiz/beryl instead of workspaces, this is an fdo standard. So, we need to add viewports support to libwnck to replace workspaces if we use compiz/beryl. If not, there is no possibility/entry in the menu to change viewport/workspace.

The move patch is needed to have the viewport support patch, they work together.

The apperance patch is only for eyecandy, it adds an apperance entry in the menu that lets the users choose the opacity/britghness/saturation of the window, as well as providing a simple way to reset all those values.

The french patch simply updates the french translation (one of the patch authors was french). This probably could have its own ticket.

"""

http://www.compiz.info/patches/libwnck/01_libwnck_window_move.patch looks good other than the existence of tabs  ;-).  We likely already have an open bug on it (with Magnus Therning filing or commenting on it; he's working on implementing it in metacity, at the very least, with the idea that he can then use libwnck to move windows)

http://www.compiz.info/patches/libwnck/02_libwnck_appearance.patch looks fine except for the fact that _NET_WM_WINDOW_OPACITY, _NET_WM_WINDOW_SATURATION, and _NET_WM_WINDOW_BRIGHTNESS are not part of the EWMH yet.  They should be pushed on wm-spec-list before we adopt the patch, IMO.

http://www.compiz.info/patches/libwnck/03_libwnck_viewport.patch has one or two spacing issues (e.g. if/else blocks should actually line up, 2 spaces after an if/else, not tabs), but other than that minor nitpick looks just fine.

http://www.compiz.info/patches/libwnck/04_libwnck_above.patch looks fine, but we should probably push a _NET_WM_ACTION_ABOVE on wm-spec-list also.

I'm not qualified to comment on the french translation.

See bug 342899, for Magnus' bug...

I was going to branch and fixup/commit some of these, but I need to get a metacity release out and then start the gnome 2.16.1 build.  So, I'll attach all the compiz patches here in case they get changed on the server (in order to make future reviews a bit easier).

Created attachment 73899 [details] [review]
01_libwnck_window_move.patch

Created attachment 73900 [details] [review]
02_libwnck_appearance.patch

Created attachment 73901 [details] [review]
03_libwnck_viewport.patch

Created attachment 73902 [details] [review]
04_libwnck_above.patch

The viewport patch adds the string "Viewport" to the visible UI. Is that something we really want? I've had to go and read the spec to find out what the difference between a viewport and a workspace is, and other than the fact that windows can overlap viewports but not workspaces I'm not really sure that there's a user-visible difference.

Would it be possible to continue referring to viewports as "Workspaces" in user-visible contexts? The only case I can think of where this would be an issue is if a user genuinely has a setup with multiple workspaces, each containing multiple viewports. In that case, falling back to mention viewports would seem reasonable.

There aren't really any inherent differences in what the user experience can be for the two EWMH concepts of workspace and viewport (*especially* with a compositing manager which can draw whatever it wants). I don't think we should confuse those EWMH implementation concepts with the currently user-visible concept of "a workspace."

The EWMH differences are entirely a matter of implementation convenience of certain features, some things are easier to code or work more smoothly if the WM is implemented one way, some are easier to code if the WM is implemented another way.

i.e. it's just two ways to implement something, like a binary tree vs. a hash table. Having this implementation choice rename things in the UI or introduce new user-visible concepts is pretty weird. You have to understand X11 and window management at a fairly deep level to even know what the heck the tradeoffs between the two implementation choices are.

The main user-visible issue is whether a window can be partly on two different workspaces at the same time (i.e. whether workspaces in the user's mental model are distinct collections of windows, or distinct areas of a conceptually large screen). But I think this user experience choice is largely independent of the implementation choice of which EWMH hint to use, esp. in a compositing-manager world.

Anyway in short if a WM does only EWMH-viewports or EWMH-workspaces I would think libwnck should display those the same way, and call them both workspaces, as Matthew suggests.

A WM could also make the user experience choice to let you nest one workspace-ish thing inside another slightly different workspace-ish thing, and try to come up with good user visible names for both workspace-ish things. I have no idea how to make this sane from a user experience standpoint, which is why metacity never did it, but assuming a libwnck patch attempts to do it, there's no reason that the language and implementation model of EWMH necessarily has to leak into the UI in the process.

Quite possibly compiz creates a better user model for showing this sort of thing. For example, perhaps you could just call EWMH-workspaces "cubes" or something like that and call EWMH-viewports "workspaces." This could work user-experience-wise, I don't know. But note that coming at it top-down like this you might not name things as they are in the protocol spec.

Does compiz really do the nesting thing though, or does it just have viewports instead?

In addition to the issues Matthew and Havoc point out about 03_libwnck_viewport.patch, let me change my mind about a few other things: 

01_libwnck_window_move.patch does not set the source indication as required in the spec.  Also, since we already have wnck_window_set_geometry(), wnck_window_move() should use it instead of writing a new _wnck_move_resize() function.  So it needs a little bit of work but is still mostly okay?

04_libwnck_above.patch: why don't we just push _NET_WM_ACTION_ABOVE on the wm-spec-list and use it instead?

last comment was on  2006-10-05 
whats the status of this?
are the patches merged yet or are do they still have some issues?

Drago: I personally find that http://bugzilla.gnome.org/show_bug.cgi?id=352383#c9 and http://bugzilla.gnome.org/show_bug.cgi?id=352383#c2 give enough explanation of what's still missing. Are there any efforts from Compiz upstreams going on wrt wm-spec-list?

01_libwnck_window_move.patch - I wrote this patch before libwnck had a set_window_geometry function. With the set_window_geometry function in place this patch is no longer useful.

02_libwnck_appearance.patch - I guess the opacity part is OK if the hint is pushed to the EWMH spec. BTW, a lot of clients change this property directly for managed windows. I believe that this should be a window/compositing manager owned property and client messages should be used by clients to change the property for a managed window, just like all other wm owned properties.

03_libwnck_viewport.patch - This patch is broken. It includes calls to wnck_window_get_workspace without checking that the result is != NULL. Changing every occurrence of

workspace = wnck_window_get_workspace (amd->window);

to something like:

if (wnck_window_is_pinned (amd->window))
    workspace = wnck_screen_get_active_workspace (wnck_window_get_screen (amd->window));
else
    workspace = wnck_window_get_workspace (amd->window);

if (!workspace)
    goto bail;

should make it better.

04_libwnck_above.patch - Yes, _NET_WM_ACTION_ABOVE and _NET_WM_ACTION_BELOW should be pushed to the EWMH spec. This patch was just a simple fix to make it work in sled10.

(In reply to comment #9)
> There aren't really any inherent differences in what the user experience can be
> for the two EWMH concepts of workspace and viewport (*especially* with a
> compositing manager which can draw whatever it wants). I don't think we should
> confuse those EWMH implementation concepts with the currently user-visible
> concept of "a workspace."

I think there are some significant differences. There's not really a limited set of viewports as there is with workspaces. The viewport can be positioned pretty much anywhere within the virtual desktop area. But if we don't expose this functionality, the currently user-visible concept of "a workspace" can be used for both.

I think everything that is useful with the viewport concept can be implemented as workspaces. Compositing manager or not doesn't matter. However, you'll end up with a lot of hacks in your WM code, like moving and changing desktop for all client windows as you change the current desktop. Most importantly, pagers and similar clients will not be aware of the viewport like functionality provided by the WM and it will end up confusing for the user. This is why I used the viewport concept when implementing the cube plugin for compiz. Clients aware of the viewport concept would just work and those that didn't could be fixed without hacking or extending the EWMH spec.

Making _NET_WM_DESKTOP (or adding a new hint that contains) a list of desktops and adding something like a _NET_WM_POSITION hint that contains a list of window positions for each desktop would make it possible to do properly implement the cube plugin using the workspace concept instead.

> 
> The EWMH differences are entirely a matter of implementation convenience of
> certain features, some things are easier to code or work more smoothly if the
> WM is implemented one way, some are easier to code if the WM is implemented
> another way.
> 
> i.e. it's just two ways to implement something, like a binary tree vs. a hash
> table. Having this implementation choice rename things in the UI or introduce
> new user-visible concepts is pretty weird. You have to understand X11 and
> window management at a fairly deep level to even know what the heck the
> tradeoffs between the two implementation choices are.

As I explained above, I think there's a bit more to it than just two implementation choices. Making sure that existing clients work reasonably well is important.

> 
> The main user-visible issue is whether a window can be partly on two different
> workspaces at the same time (i.e. whether workspaces in the user's mental model
> are distinct collections of windows, or distinct areas of a conceptually large
> screen). But I think this user experience choice is largely independent of the
> implementation choice of which EWMH hint to use, esp. in a compositing-manager
> world.

The EWMH hint used only makes it possible for other clients to be aware of the WM state and manipulate it to some degree. To me it makes sense to pick the hint that gives the most accurate view of real WM state. Whether the WM lives in a compositing-manager world or not doesn't really matter, it just makes the possible WM states much greater.

> 
> Anyway in short if a WM does only EWMH-viewports or EWMH-workspaces I would
> think libwnck should display those the same way, and call them both workspaces,
> as Matthew suggests.

I agree. In the viewport case we should just split the large desktop area in screen sized chunks (like my viewport patch is doing) and call them workspaces.

> 
> A WM could also make the user experience choice to let you nest one
> workspace-ish thing inside another slightly different workspace-ish thing, and
> try to come up with good user visible names for both workspace-ish things. I
> have no idea how to make this sane from a user experience standpoint, which is
> why metacity never did it, but assuming a libwnck patch attempts to do it,
> there's no reason that the language and implementation model of EWMH
> necessarily has to leak into the UI in the process.
> 
> Quite possibly compiz creates a better user model for showing this sort of
> thing. For example, perhaps you could just call EWMH-workspaces "cubes" or
> something like that and call EWMH-viewports "workspaces." This could work
> user-experience-wise, I don't know. But note that coming at it top-down like
> this you might not name things as they are in the protocol spec.
> 
> Does compiz really do the nesting thing though, or does it just have viewports
> instead?

The core of compiz allows both things to exist. However, there's no plugin afaik that creates a visual representation of multiple workspaces and switching workspace without that will just look like switching workspace in a normal wm so most people don't use it. A plugin that visualizes workspaces as multiple cubes will probably be available soon and people will likely start using it then.

Created attachment 81510 [details] [review]
_NET_WM_STATE_ABOVE patch

This should fix the spacing problems with the _NET_WM_STATE_ABOVE patch.

Created attachment 81511 [details] [review]
viewport patch

Changes:
  * use "Workspace" instead of "Viewport" in strings
  * NULL check on the workspace variable
  * only show "Move to Another Workspace" if you don't have a window pinned
  * fix spacing problems

Hopefully this is good to commit now.

Oh, forgot to mention the viewport patch uses wnck_window_set_geometry now so the wnck_window_move patch is no longer needed.

I don't think the Appearance patch is worth having so I'm not going to do work on it.

*bump*

Can someone take a look at the updated patches?

I am using your _NET_WM_STATE_ABOVE.patch since you uploaded it here and I never had any problem with it. 
I don't see any reasons why this patches shouldn't go in.

The right thing to do here is still to get NET_WM_ACTION_ABOVE in the EWMH...

http://lists.freedesktop.org/archives/xdg/2007-March/009562.html

discussion from comment 21 got moved to:
http://mail.gnome.org/archives/wm-spec-list/2007-March/msg00016.html

Created attachment 86190 [details] [review]
Add support for ACTION_ABOVE and ACTION_BELOW

As discussed on compiz and wm-spec lists as well as here, I think the right fix is just getting ACTION_ABOVE into the EWMH and then supporting it, rather than working around the fact that window managers can't yet advertise it.  Since Matthias already proposed this EWMH change a couple weeks ago, I'm going ahead and commiting the fix now and will modify later if someone objects to the proposal soon on wm-spec-list.

Travis: the patches modifying window-action-menu.c might need an update for trunk.

Created attachment 87961 [details] [review]
Get/set opacity/saturation/brightness support for windows

This is patch 73900 updated to trunk, and without the menu items added to the action menu. I don't think we want those menu items. However the new API can be useful.

Elijah: my only worry is that this uses _NET_WM_WINDOW_OPACITY _NET_WM_WINDOW_SATURATION and _NET_WM_WINDOW_BRIGHTNESS which are not in EWMH yet, I believe. Should they be added to the spec first?

(In reply to comment #26)
> Elijah: my only worry is that this uses _NET_WM_WINDOW_OPACITY
> _NET_WM_WINDOW_SATURATION and _NET_WM_WINDOW_BRIGHTNESS which are not in EWMH
> yet, I believe. Should they be added to the spec first?
 
Yes, I still think so (see comment 2) and David also mentioned as much (see comment 13).

Add ewmh to whiteboard to indicate that we need some work in EWMH for this bug.

I'd really love to have someone update the viewport patch.

I'm not quite sure if (and how) we should handle the case when the wm is using a viewport and workspaces, though.

Created attachment 91081 [details] [review]
Updated viewport patch

Viewport patch that applies to latest SVN

Thanks for the update Robert!

We still need to decide what to do when there are both workspaces and viewports.

Can't we just declare that an unsupported configuration ? I mean,
do we really have any window manager which support such configurations ?

Compiz supports both but using both is not something I can imagine someone doing.

(In reply to comment #32)
> Can't we just declare that an unsupported configuration ? 

+1 from me.

Created attachment 91420 [details] [review]
New patch for viewport support in the action menu

Ok, since it doesn't make sense to have both at the same time, it also doesn't make sense to create menu items for viewports and workspaces.

Now, the actions are doing workspace stuff if there's more than one workspace and viewport stuff if there's only one workspace.

Can someone with compiz test the patch to see if it works?

(of course, ignore my TODO list that's at the top of test-tasklist.c ;-))

I have just tried this using Compiz and libwnck svn.
The "Only on this workspace/Always visible" radios aren't working correctly (it is always "Always visible" whatever I do, while it isn't on all viewports)
The workspace switcher is somehow working, but current vieport is always the "Viewport 1" and the other viewports are considered relatively to the current viewport (e.g. if I'm on Viewport 2 and selected "Move to Viewport 3" the window is moved to Viewport 4)

Created attachment 91523 [details] [review]
wnck-viewports-support.patch

Okay, my bad for the radios button, I hadn't understood how it worked (by the way, I think that the behavior of the pin/unpin button should be reversed and reflect current situation, not what it would change, since it's a radio button ; but that's off topic)

I looked at the patch and I guess I fixed it (at least it now works perfectly on my setup). Current viewport is now detected properly (using wnck_workspace_get_viewport_x/y) and move up/down/left/right actions are correctly available (with the previous patch only the right one was ever available since it wasn't detecting current viewport correctly). Move to workspace was a bit harder to fix. First it looks like using wnck_window_get_client_window_geometry instead of wnck_window_get_geometry is needed, because of some Compiz oddities when it comes to decorations handling (otherwise the "content" window left corner is moved at the position of the "content + decorations" window left corner, just exactly why one shouldn't use _get_client_window_geometry usually (as far as what I have read in the _get_geometry function comments)). Then wnck_window_set_geometry apparently works relatively to the current viewport so substracting current viewport x/y to destination x/y was also needed (it looks like it is more a move function than an absolute function).

Guillaume: thanks for the update. However, I fixed the patch in the train yesterday ;-) I'll commit it soon.

(In reply to comment #39)
> Guillaume: thanks for the update. However, I fixed the patch in the train
> yesterday ;-) I'll commit it soon.

Committed.

-- 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/71.