view: Add view_move_to_front/back() with _no_focus variant

[jlindgren90]

This avoids calling view->impl functions from cursor.c and desktop.c.

No functional change intended.

[Consolatis]

I am not sure about moving _to_front/back to view.c.
Usually we do all actions relating to interactions of different views in desktop.c and actions relating to views themselves in view.c.

An alternative solution to the PR could be to return early if the view is already the top view, so we wouldn't call cursor_update_focus() if the view is already at the top.

Edit:
Although that could get a bit tricky with xwayland views and their sub views.
So maybe something like this in desktop_move_to_front/back?

struct view *top_view = wl_container_of(server->views.prev, top_view, link);
if (view->impl->move_to_front) {
    view->impl->move_to_front(view);
}
if (top_view != wl_container_of(server->views.prev, top_view, link)) {
    cursor_update_focus(server);
}

[jlindgren90]

Usually we do all actions relating to interactions of different views in desktop.c and actions relating to views themselves in view.c.

Yes, this is an awkward case though since the actual multi-view interactions are done in the view->impl functions, not in desktop.c. So the conceptual separation is already broken in that sense.

[johanmalm]

I'm good with moving these to view.c and on reflection, do think it's actually preferable.

If we wanted to be really hard on the desktop argument, we should have left all the xwayland-move-sub-view stuff in desktop.c --- but I think it is better the way we've gone (even if it's adds a bit of grey to the desktop/view division).

The way John has proposed keeps the calling of view->impl->* more consistent.

An alternative solution to the PR could be to return early if the view is already the top view, so we wouldn't call

Another problem with this is that we want to be workspace specific (i.e. scene_tree orientated rather than iterating over server->views).

On that note, I don't think there are any places left in the code that cares about the order the server->views other than xwayland's move_sub_views().
So we could nearly remove the

wl_list_remove(&view->link);
wl_list_insert(&view->server->views, &view->link);

...which helps it feel even less 'desktopy'

Whilst on the topic - do we actually need the move_sub_views() thing? I tried with openbox/mutter and they don't seems to let focus change to other toplevel with popups/sub-windows open at all. I found this when experimenting the close_all_popups PR. We could do with finding an xwayland application where the whole move_sub_views() actually makes a difference. I must have put it in there for a reason, but I can't remember what it is.

[Consolatis]

I think the move_sub_views() thingie was about A-Tab so it would focus dialog windows and similar that are on top of the main window.

On that note, I don't think there are any places left in the code that cares about the order the server->views other than xwayland's move_sub_views(). So we could nearly remove the

wl_list_remove(&view->link);
wl_list_insert(&view->server->views, &view->link);

I actually intend to revert to using server->views + filter for current workspace (at least for A-Tab) because otherwise always-on-top views (and other non direct children of the workspace nodes) will not show up there (#856).

Edit:
I am fine with the movement to view.c.

[jlindgren90]

Whilst on the topic - do we actually need the move_sub_views() thing? I tried with openbox/mutter and they don't seems to let focus change to other toplevel with popups/sub-windows open at all.

move_sub_views() is needed with any X11 application that uses floating toolbars or the like (a UI pattern that incidentally does not seem to really be allowed for native Wayland applications). Try Audacious in Winamp mode - the playlist & equalizer windows should rise to the top along with the main window.

[jlindgren90]

v2: Add an explicit recursion guard in cursor_update_focus().

[Consolatis]

Sweet, just looked through the diff again to verify that view is always != NULL when calling due to the new assert(view).
This PR looks good to me but I didn't test. I really like the recursion guard better than having the 2 different code paths.

[johanmalm]

LGTM. Ready for merge?

[johanmalm]

I actually intend to revert to using server->views + filter for current workspace (at least for A-Tab) because otherwise always-on-top views (and other non direct children of the workspace nodes) will not show up there (#856).

I had been thinking similar thoughts 😄
+possibly showing all workspaces in windowSwitcher? I know we said originally that we wouldn't but...
Preview would be hard I guess - but possibly not needed for other workspaces.