Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: tristate toggle logic for tab visibilty #5530

Merged

Conversation

pajlada
Copy link
Member

@pajlada pajlada commented Aug 4, 2024

This removes the "toggle live" option for tab visibilty options, including the hotkey. so this tries to remove the hotkey if it hasn't been touched by the user

This changes the tab visibilty options in the menu from "toggle" & "toggle live" to "show all", "show live only", and "hide all" in a submenu

This removes all tab visibility options for the emote popup

For previous context, see #5353

The existing toggle option will reset the live only filter, and switch between "hide all" and "show all"

Fixes #5341

Copy link
Contributor

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be good to know if the removed hotkey was used.

This PR should also mention #5341. One thing that gets lost is the visibility of the toggle hotkey inside the context menu.

Now the state graph looks like this:

graph TD;
    All-->|liveOnly|LiveOnly
    LiveOnly-->|on|All
    LiveOnly-->|toggle|None
    LiveOnly-->|liveOnly|LiveOnly
    LiveOnly-->|off|None
    All<-->|toggle|None
    All-->|off|None
    All-->|on|All
    None-->|off|None
    None-->|liveOnly|LiveOnly
Loading


for (int index = 0; index < this->items_.size(); ++index)
{
T item = this->items_[index];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This copies all items, but only the to-be-removed item should be copied.

src/controllers/hotkeys/ActionNames.hpp Outdated Show resolved Hide resolved
src/controllers/hotkeys/HotkeyController.cpp Outdated Show resolved Hide resolved
@pajlada
Copy link
Member Author

pajlada commented Aug 4, 2024

It would probably be good to know if the removed hotkey was used.

Anecdotally, I've only heard of people using the removed hotkey accidentally and wondering why some of their tabs are missing

Mm2PL
Mm2PL previously requested changes Aug 5, 2024
src/widgets/Window.cpp Outdated Show resolved Hide resolved
src/controllers/hotkeys/HotkeyController.cpp Show resolved Hide resolved
src/widgets/Window.cpp Outdated Show resolved Hide resolved
src/widgets/Window.cpp Outdated Show resolved Hide resolved
@pajlada pajlada requested a review from Mm2PL August 10, 2024 13:32
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/controllers/hotkeys/HotkeyController.cpp Show resolved Hide resolved
src/controllers/hotkeys/HotkeyController.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/controllers/hotkeys/HotkeyController.cpp Show resolved Hide resolved
src/controllers/hotkeys/HotkeyController.cpp Show resolved Hide resolved
@pajlada pajlada dismissed Mm2PL’s stale review August 24, 2024 09:18

Feedback fixed

@pajlada pajlada enabled auto-merge (squash) August 24, 2024 09:19
Copy link
Member Author

@pajlada pajlada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

}

void HotkeyController::warnForRemovedHotkeyActions(HotkeyCategory category,
QString action,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: the parameter 'action' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

src/controllers/hotkeys/HotkeyController.hpp:146:

-     void warnForRemovedHotkeyActions(HotkeyCategory category, QString action,
+     void warnForRemovedHotkeyActions(HotkeyCategory category, const QString& action,
Suggested change
QString action,
const QString& action,


void HotkeyController::warnForRemovedHotkeyActions(HotkeyCategory category,
QString action,
std::vector<QString> args)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: the parameter 'args' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

src/controllers/hotkeys/HotkeyController.hpp:147:

-                                      std::vector<QString> args);
+                                      const std::vector<QString>& args);
Suggested change
std::vector<QString> args)
const std::vector<QString>& args)

@pajlada pajlada merged commit 5170085 into master Aug 24, 2024
17 checks passed
@pajlada pajlada deleted the fix/align-tab-visibility-to-some-logic-that-might-not-be-logical branch August 24, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggle visibility of tabs hotkey behaves differently than right click when used with Show live tabs only
3 participants