feature: alt + grave behavior now implemented alongside alt + tick #3216

mattkae · 2024-02-05T21:38:53Z

What's new?

I realized that the last PR was going to be messy, as we would have to maintain a tab order for every application and every window.
However, I realized that this list was one in the same. Basically, we just keep the order of Windows, and we choose whether or not we are jumping from App to App or from Window to Window within an app
So much simpler, less data to keep track of, and easier to read 🪄

AlanGriffiths

I've not yet reviewed the functionality, but there's a bunch of "housekeeping" that can be cleared away already.

AlanGriffiths · 2024-02-06T14:26:55Z

include/miral/miral/minimal_window_manager.h

+    void advise_new_window(WindowInfo const& app_info) override;
+
+    void advise_delete_window(WindowInfo const& app_info) override;
+


We should document when these are introduced.

Suggested change

void advise_new_window(WindowInfo const& app_info) override;

void advise_delete_window(WindowInfo const& app_info) override;

/// \remark Since MirAL 5.0

void advise_new_window(WindowInfo const& app_info) override;

/// \remark Since MirAL 5.0

void advise_delete_window(WindowInfo const& app_info) override;

debian/libmiral7.symbols

AlanGriffiths · 2024-02-06T14:28:35Z

include/miral/miral/window_manager_tools.h

+    /// Check if the provided window can be selected
+    auto can_select_window(Window const&) const -> bool;


Suggested change

/// Check if the provided window can be selected

auto can_select_window(Window const&) const -> bool;

/// Check if the provided window can be selected

/// \remark Since MirAL 5.0

auto can_select_window(Window const&) const -> bool;

AlanGriffiths · 2024-02-06T14:32:36Z

src/miral/application_selector.cpp

    // If we delete the selected application, we will try to select the next available one.
-    if (application == selected.lock())
+    if (*it == selected)


The code no longer agrees wit hthe comment

AlanGriffiths · 2024-02-06T14:36:10Z

src/miral/symbols.map

+    miral::MinimalWindowManager::advise_new_window*;
+    non-virtual?thunk?to?miral::MinimalWindowManager::advise_new_window*;
+    miral::MinimalWindowManager::advise_delete_window*;
+    non-virtual?thunk?to?miral::MinimalWindowManager::advise_delete_window*;


We don't need a new stanza - we haven't released Miral 5.0 yet. (Just run the regenerate script)

…ame app multiple times in a row

codecov · 2024-02-06T20:15:37Z

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (3a751d3) 77.81% compared to head (3f87567) 77.84%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/miral/application_selector.cpp	74.28%	18 Missing ⚠️
src/miral/window_management_trace.cpp	0.00%	6 Missing ⚠️
src/miral/minimal_window_manager.cpp	60.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3216      +/-   ##
==========================================
+ Coverage   77.81%   77.84%   +0.03%     
==========================================
  Files        1072     1072              
  Lines       75072    75154      +82     
==========================================
+ Hits        58415    58503      +88     
+ Misses      16657    16651       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AlanGriffiths

This looks like a sensible implementation, and works sensibly in the scenarios I've tried.

However, there don't seem to be enough tests. I'd expect a similar number of new tests for within-app switching as we have existing tests for between-app switching. Am I missing something?

AlanGriffiths · 2024-02-07T10:22:01Z

src/miral/application_selector.h

-    Window active_window;
+    Window selected;
+
+    bool _is_active = false;


We don't use "_" at the beginning of variables. (I thought the guide said something about this, but I don't see it now.) Anyway, once upon a time some library vendors assumed that the _[A-Z].+ reserved identifiers allowed macros starting with "_". (It is probably no longer an issue, but let's be consistent with the codebase.)

Suggested change

bool _is_active = false;

bool is_active_ = false;

AlanGriffiths · 2024-02-07T12:58:46Z

debian/libmiral7.symbols

+ (c++)"miral::WaylandExtensions::Context::Context()@MIRAL_5.0" 5.0.0
+ (c++)"miral::WaylandExtensions::Context::~Context()@MIRAL_5.0" 5.0.0


Huh?! (I can't imagine why you're adding this - it should have been there for a long time already.)

When I regenerated the symbols, it got generated too

Hence the "Huh?!"

…oved" This reverts commit fce8ce9, reversing changes made to 1032aed.

Saviq · 2024-02-15T13:30:56Z

@mattkae had to revert this, as it made Frame fail to start up:

Mir fatal error: advise_focus_gained: window was not found in the list
!!! Fatal signal received. Attempting cleanup, but deadlock may occur
Mir fatal error: Unsupported attempt to continue after a fatal signal: SIGABRT
[1]    36090 IOT instruction (core dumped)  ubuntu-frame

EDIT:
--wallpaper=false works around, so whatever the wallpaper client does isn't handled here.

EDIT2:
Ah no, starting a client without the wallpaper results in the same. So it must be Frame's special window management.

mattkae · 2024-02-15T14:06:50Z

@mattkae had to revert this, as it made Frame fail to start up:
Mir fatal error: advise_focus_gained: window was not found in the list
!!! Fatal signal received. Attempting cleanup, but deadlock may occur
Mir fatal error: Unsupported attempt to continue after a fatal signal: SIGABRT
[1]    36090 IOT instruction (core dumped)  ubuntu-frame
EDIT: --wallpaper=false works around, so whatever the wallpaper client does isn't handled here.

EDIT2: Ah no, starting a client without the wallpaper results in the same. So it must be Frame's special window management.

Ah that is no good! Anything I can do to help on frame's side of things? Spooky that the window was not found in the list... probably not calling a super method?

AlanGriffiths · 2024-02-15T14:32:29Z

Ah that is no good! Anything I can do to help on frame's side of things? Spooky that the window was not found in the list... probably not calling a super method?

Maybe you can track that down?

mattkae · 2024-02-15T14:46:30Z

Ah that is no good! Anything I can do to help on frame's side of things? Spooky that the window was not found in the list... probably not calling a super method?

Maybe you can track that down?

Yup! I'll look into it now

…ick-improved"" This reverts commit 74d847e.

Revert "Revert "Merge pull request #3216 from MirServer/feature/alt-t…

mattkae requested a review from AlanGriffiths February 5, 2024 21:38

feature: alt + grave behavior now implemented alongside alt + tick

a00a1aa

mattkae force-pushed the feature/alt-tick-improved branch from 9f85fed to a00a1aa Compare February 5, 2024 22:11

AlanGriffiths requested changes Feb 6, 2024

View reviewed changes

mattkae added 2 commits February 6, 2024 11:03

alt-tick: pull request feedback

9162e0a

bugfix: fix for the application selector accidentally selecting the s…

7f3ec9b

…ame app multiple times in a row

mattkae requested a review from AlanGriffiths February 6, 2024 19:08

AlanGriffiths requested changes Feb 7, 2024

View reviewed changes

alt tick: pr feedback

3f87567

AlanGriffiths approved these changes Feb 8, 2024

View reviewed changes

AlanGriffiths added this pull request to the merge queue Feb 8, 2024

Merged via the queue into main with commit fce8ce9 Feb 8, 2024
22 of 24 checks passed

AlanGriffiths deleted the feature/alt-tick-improved branch February 8, 2024 11:19

AlanGriffiths mentioned this pull request Feb 13, 2024

Excessive CPU usage #3230

Open

AlanGriffiths added a commit that referenced this pull request Feb 13, 2024

Revert "Merge pull request #3216 from MirServer/feature/alt-tick-impr…

e9343ea

…oved" This reverts commit fce8ce9, reversing changes made to 1032aed.

Saviq added a commit that referenced this pull request Feb 15, 2024

Revert "Merge pull request #3216 from MirServer/feature/alt-tick-impr…

74d847e

…oved" This reverts commit fce8ce9, reversing changes made to 1032aed.

mattkae added a commit that referenced this pull request Feb 15, 2024

Revert "Revert "Merge pull request #3216 from MirServer/feature/alt-t…

2a35d7b

…ick-improved"" This reverts commit 74d847e.

github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024

Merge pull request #3234 from canonical/bugfix/alt-tab

91fddda

Revert "Revert "Merge pull request #3216 from MirServer/feature/alt-t…

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: alt + grave behavior now implemented alongside alt + tick #3216

feature: alt + grave behavior now implemented alongside alt + tick #3216

mattkae commented Feb 5, 2024

AlanGriffiths left a comment

AlanGriffiths Feb 6, 2024

AlanGriffiths Feb 6, 2024

AlanGriffiths Feb 6, 2024

AlanGriffiths Feb 6, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths Feb 7, 2024

AlanGriffiths Feb 7, 2024

mattkae Feb 7, 2024

AlanGriffiths Feb 7, 2024

Saviq commented Feb 15, 2024 •

edited

Loading

mattkae commented Feb 15, 2024

AlanGriffiths commented Feb 15, 2024

mattkae commented Feb 15, 2024

		void advise_new_window(WindowInfo const& app_info) override;

		void advise_delete_window(WindowInfo const& app_info) override;

		/// Check if the provided window can be selected
		auto can_select_window(Window const&) const -> bool;

		(c++)"miral::WaylandExtensions::Context::Context()@MIRAL_5.0" 5.0.0
		(c++)"miral::WaylandExtensions::Context::~Context()@MIRAL_5.0" 5.0.0

feature: alt + grave behavior now implemented alongside alt + tick #3216

feature: alt + grave behavior now implemented alongside alt + tick #3216

Conversation

mattkae commented Feb 5, 2024

What's new?

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2024 • edited Loading

Codecov Report

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saviq commented Feb 15, 2024 • edited Loading

mattkae commented Feb 15, 2024

AlanGriffiths commented Feb 15, 2024

mattkae commented Feb 15, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

Saviq commented Feb 15, 2024 •

edited

Loading