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

feat: Warn for commands with duplicate triggers #4322

Merged

Conversation

askepticaldreamer
Copy link
Contributor

@askepticaldreamer askepticaldreamer commented Jan 21, 2023

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Fixes #2497
Added warning tooltip for when user selected another cell and previous command name was a duplicate. Tried to force selection to the duplicate cell with the following

  int duplicateIndex = (i > previous.row()) ? i : previous.row();
  view->getTableView()->selectRow(duplicateIndex);
  view->getTableView()->selectColumn(0);

But didn't seem to work. Could leave as is with just tool tip warning or open to suggestions for methods to force user to change duplicate command name

@pajlada
Copy link
Member

pajlada commented Jan 21, 2023

I think the friendliest way to handle this atm will be to not stop the user from renaming or creating commands with specific names, but to mark all commands with duplicate entries. This can be done by making them slightly yellow, or adding a ⚠️ icon to the start of the row.
In addition, a warning text should appear above or below the list if any of the commands have duplicate names

@pajlada pajlada marked this pull request as draft January 21, 2023 19:52
@pajlada
Copy link
Member

pajlada commented Jan 21, 2023

Marked it as a draft until it solves it (or you think it solves the issue), feel free to poke me at any time before that if you want a review of the code or way you're trying to handle it

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

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/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated 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/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
@askepticaldreamer
Copy link
Contributor Author

@pajlada what do you think of this approach? Am I overcomplicating things? The code is able to set text colors and add/remove the warning label as duplicates are added/removed but currently runs into an issue when it tries to set the color after removing an item or moving it up/down so not sure if I need a mutex somewhere or something.

Copy link
Member

@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.

Looks alright, let's review in parts so you can work on these things first

src/widgets/settingspages/CommandPage.hpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated 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/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
src/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
Copy link
Member

@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.

With this PR I'm able to crash Chatterino by:

  1. Removing the last row (maybe removing any row?)
  2. Dragging one row onto another

@pajlada
Copy link
Member

pajlada commented Feb 12, 2023

I don't think a button for this is the right approach - if it cannot be figured out automatically then I don't think it provides much value for users

@pajlada
Copy link
Member

pajlada commented Feb 12, 2023

If it's just there for you while testing then my bad, bit fast with the comments 😎

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/widgets/settingspages/CommandPage.cpp Outdated Show resolved Hide resolved
@pajlada
Copy link
Member

pajlada commented Feb 14, 2023

Doesn't seem to be checking duplicates on row removal, and doesn't handle renaming from one duplicate to another duplicate well
https://user-images.githubusercontent.com/962989/218690818-94a129cd-91df-45b9-87c5-09937e4e6dfe.mp4

@askepticaldreamer
Copy link
Contributor Author

@pajlada that's interesting, both those cases work on my machine. I'll record a video when I get a chance. Should be fun to debug lol.

…serted, rowsRemoved, and dataChanged signals
@askepticaldreamer
Copy link
Contributor Author

Was able to repro. Wasn't happening before since I was testing duplicate commands that weren't the one just added. Changed the signals and seems to work now

@pajlada pajlada marked this pull request as ready for review March 16, 2024 13:34
Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

lgtm

@pajlada pajlada changed the title Handle duplicate command names feat: Warn for commands with duplicate triggers Mar 17, 2024
@pajlada pajlada enabled auto-merge (squash) March 17, 2024 11:23
@pajlada pajlada merged commit 46c5609 into Chatterino:master Mar 17, 2024
17 checks passed
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.

It is possible to have two commands named the same
3 participants