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

Change maskeditor undo key combo #2915

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

silveroxides
Copy link
Contributor

@silveroxides silveroxides commented Mar 7, 2025

Previous one conflicted with the litegraph node canvas undo action and did not have any effect. Alt key when used in combination with these keypresses should not have any conflict with browser or frontend.

┆Issue is synchronized with this Notion page by Unito

Previous one conflicted with the litegraph node canvas undo action and did not have any effect. Alt key when used in combination with these keypresses should not have any conflict with browser or frontend.
@silveroxides silveroxides requested review from trsommer and a team as code owners March 7, 2025 18:48
@trsommer
Copy link
Collaborator

trsommer commented Mar 8, 2025

I would propose the keybinding system should be changed instead to allow for the same keybinding in different zones to do different things. It is not logical for undo to be ctrl + z in one place and alt + z in another.

Another problem I have is that alt + z opens the nvidia overlay on windows and is blocked from reaching the browser

image

@@ -4919,15 +4919,15 @@ class KeyboardManager {
// combinations

private undoCombinationPressed() {
const combination = ['ctrl', 'z']
const combination = ['alt', 'z']
Copy link
Contributor

Choose a reason for hiding this comment

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

This will conflict with any user that has already bound alt + z to something else.

To resolve this without fixing the underlying issues (contexttual shortcuts & global ctrl + z), the key bindings need to be configurable, with the defaults unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well right now it doesn't even work at all so. Also there isn't even proper undo queue so that switching between the brush, eraser or "color"Selector initiate a new queue and a separated instance in the editor. There are so many basic things that are completely overlooked. Numerical representations for sliders, tooltip or explanation what the difference is between simple/hsl/lab, explanation for the "mask tolerance", option for feathering as only having tolerance will result in masks either being too small or select way too much. Ever since inpainting was introduced, we were told that better mask more than too little. Also a color selector (often referred to as color picker) is completely different thing. Especially when using pipette icon. It is meant to get the id of a color so that it can be selected in a brush palette.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would need to be split into multiple PRs.

@silveroxides
Copy link
Contributor Author

@trsommer Nvidia overlay keybind can easily be configured to anything else.

@trsommer
Copy link
Collaborator

trsommer commented Mar 9, 2025

  1. It is true that it doesn't work at the moment and needs fixing.
  2. There is a proper undo queue. Every tool action can be reversed, just like in any other image editor like Photoshop. No new queue is initialized; this is false.
  3. It is true that there are many things missing. I invite you to create them and submit pull requests so they can be implemented. Things like numerical representation of slider values or tooltips are easy projects that shouldn't take too long.
  4. I agree. There needs to be better documentation on what the features do. Simple/HSL/LAB are different ways of comparing colors.
  5. I don't agree with your criticism of the pipette icon. This is not an image editor at the moment, so selecting colors using a color picker to create a mask based on the colors of the image seems quite reasonable to me. However, I am open to suggestions to improve it.
  6. I do not agree that the user should just rebind the NVIDIA overlay. Most users are probably using ComfyUI on Windows and have an NVIDIA graphics card. You can't expect most of your users to rebind a commonly used keybinding just because you need it for some other function.

To really solve the keybinding issue, I invite you to look at this PR #1820 and this branch [keyboard-shortcuts-rewrite] I opened a few months ago. It directly addresses the problem you describe. It had other problems, but they can be fixed. I just did not have the time.

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.

3 participants