gabm
pressing a digit key selects a color out of the palette
Pressing 1, 2, 3, …, 0 (meaning 10) selects the nth color from the palette.
This is not reflected in bottom toolbar though; not sure how to do it yet. Pointers appreciated.
Also if this happens to be a “we are definitely not merging this”, tell me also, so that I can focus on the behavior that I need and close the pr.
this is in my attempt to make the style toolbar reflect the currently selected color, but it hasn't helped. I worry it is some gtk magic I cannot hope to comprehend
in less cryptic words: what I think is happening, is when you click on a button, some place (this is what I'm trying to find) is updating how the toolbar looks (different color button now highlighted), and then sends a StyleToolbarInput::ColorButtonSelected to actually update the currently used color.
I'm not sure if there's a way to send the “update how the toolbar looks please 😌” event from where I'm handling the key event. Would love to hear advice / feedback on this.
If it's not something I can do in practical terms, it might be reasonable to switch back to the simpler implementation one commit ago.
Apologies, I'm not finding much time for proper reviews right now. I'll provide better feedback later when I find time, but that might be several days.
Initially, I thought we'd use the number keys for zoom (e.g. 1 -> 100%, 2 -> 200% / Shift+2 -> 50%, etc...) along the line of what GIMP does. But I realise that single key shortcuts may clash with configured shortcuts for tools. So perhaps for zoom, we'll use something different down the road, e.g. ctrl++, ctrl+-, etc. So using digits here should be fine.
My thoughts for now: The digits can clash with configured shortcuts. E.g. I could configure
marker = "4"
and that would override the color palette keybinding. I think my preference would be:
- ~limit the number of palette colors to 10 (if we haven't already)~
- have default but overridable bindings along the line of
[keybinds]
...
color1 = "1"
color2 = "2"
color3 = "3"
...
color10 = "0"
- in sketchboard, avoid
else if let ... else if letand handle both toolbar and style toolbar shortcuts in one go
Yes, this may be more static and less elegant, but it would avoid a configurable clash.
Regarding the button activation, afaict right now, handling of ToolsToolbarInput::SwitchSelectedTool(tool) (as an example) sets an active button here, something similar may be necessary to have a UI reflection of the selected color. I haven't tried it now, but I think it should somehow be possible.
No worries, several days is still beyond reasonable, especially compared to the projects I usually contribute to. Take your time.
Something somewhat unique about satty, is that we have a very visible, indexable list of colors — this is not usually something clear and visible in the ui of something like gimp or krita.
So numbers suddenly not targetting that but targetting zoom levels feels like a missed opportunity, for that reason.
ctrl+= and ctrl+- sounds a lot more fitting for satty, but the idea I had is to just have - and + for zoom, and = to reset zoom to 100%
Very simple and even guessable mappings, that are a bit nicer to press than ctrl+= / ctrl+-.
My thoughts on these color indexing hotkeys possibly colliding with user configured tool shortcuts:
Currently, letter keys are essentially free range for the user to use for tools;
They are all free, so it doesn't make a lot of sense for the user to put tools on the less accessible keys (the digit keys).
So I think the possible colliding is a non-issue actually.
We really shouldn't limit the number of allowed palette colors. Please. I beg you!
Being able to set more than 10 palette colors is a very big “omg this is so nice” that I felt when discovering satty.
We just make it so only the first 10 are accessible with hotkeys, and the other ones are only accessible with the mouse.
We certainly shouldn't limit the user just to make the 10 hotkeys “make sense”.
Making the hotkeys for the colors configurable is not a bad idea, but I'm wondering if it's rushed.
For example, the Delete key clears — would be cool if it was configurable, but ultimately it's such a reasonable default that there's not much value in it being configurable.
The user goes through the docs for the first time, sees “Delete to clear”, goes “yeah that makes sense”, and continues on.
Similarly here, 1, …, 9, 0 picking colors is also very straightforward; where I'm not sure it's worth my effort to make them changeable to something else.
tldr: I'm :/ about it, but open to hear if you really are certain that it's necessary. I personally think it is solving a non-existent problem.
avoid else if let ... else if let
Not sure what you mean by this yet, but maybe it will make sense when I look at the code again.
About button activation, I'll try to follow this lead and see if it takes me anywhere, thank you.
I'll ask ahead of time: if I can't figure out how to make the ui reflect the selected color / it's impossible, do you see this pr being merged regardless? Or would this be a blocker?
Asking this so that I can direct my effort towards solving it, and give up completely if I can't figure it out.
Rather than first waste my effort on making configurable colors (possibly), only for the pr to be left ❌ when ui updates turn out to be a no-go.
Something somewhat unique about satty, is that we have a very visible, indexable list of colors — this is not usually something clear and visible in the ui of something like gimp or krita. So numbers suddenly not targetting that but targetting zoom levels feels like a missed opportunity, for that reason. ctrl+= and ctrl+- sounds a lot more fitting for satty, but the idea I had is to just have - and + for zoom, and = to reset zoom to 100% Very simple and even guessable mappings, that are a bit nicer to press than ctrl+= / ctrl+-.
That's definitely nicer to use. I'm a bit worried about shortcuts not accessible while editing text (or perhaps when using other tool specific shortcuts, which could also become more in the future). Perhaps there just needs to be a way to toggle between program global and tool specific shortcuts, but at the same time it could be confusing. But this needs another issue to discuss, I think.
My thoughts on these color indexing hotkeys possibly colliding with user configured tool shortcuts: Currently, letter keys are essentially free range for the user to use for tools; They are all free, so it doesn't make a lot of sense for the user to put tools on the less accessible keys (the digit keys). So I think the possible colliding is a non-issue actually.
Well, we did have digit shortcuts (in addition to the letter shortcuts) as part of the original shortcut draft PR #148, so somebody had that thought.
We really shouldn't limit the number of allowed palette colors. Please. I beg you! Being able to set more than 10 palette colors is a very big “omg this is so nice” that I felt when discovering satty. We just make it so only the first 10 are accessible with hotkeys, and the other ones are only accessible with the mouse. We certainly shouldn't limit the user just to make the 10 hotkeys “make sense”.
Yeah, fine with me. I just didn't think it was a use case.
Making the hotkeys for the colors configurable is not a bad idea, but I'm wondering if it's rushed. For example, the Delete key clears — would be cool if it was configurable, but ultimately it's such a reasonable default that there's not much value in it being configurable. The user goes through the docs for the first time, sees “Delete to clear”, goes “yeah that makes sense”, and continues on. Similarly here, 1, …, 9, 0 picking colors is also very straightforward; where I'm not sure it's worth my effort to make them changeable to something else. tldr: I'm :/ about it, but open to hear if you really are certain that it's necessary. I personally think it is solving a non-existent problem.
I'm happy to wait for future issues + PRs regarding this. But if we do leave it for later, it would be nice if we could print a warning if anyone tries to bind a digit as a tool shortcut in the meantime.
avoid else if let ... else if let
Not sure what you mean by this yet, but maybe it will make sense when I look at the code again.
That's obsolete now with last paragraph :smile:
About button activation, I'll try to follow this lead and see if it takes me anywhere, thank you. I'll ask ahead of time: if I can't figure out how to make the ui reflect the selected color / it's impossible, do you see this pr being merged regardless? Or would this be a blocker? Asking this so that I can direct my effort towards solving it, and give up completely if I can't figure it out. Rather than first waste my effort on making configurable colors (possibly), only for the pr to be left ❌ when ui updates turn out to be a no-go.
It would definitely look more polished if the UI was updated. If you get stuck, I'm happy to take a closer look, see if I can make a better suggestion, or perhaps just add a commit. If it still doesn't work then I don't think it's a deal breaker, but we should at least try and I'm fairly sure either of us can figure it out.
Just a heads up, after having a slightly closer look at PR #343, that one also deals with key events in sketch_board.rs. This might cause a merge conflict then.
shortcuts not accessible while editing text
Ohhh hmm didn't think of that actually. one one hand zooming while editing text may be too niche to consider, but the “or other tools' shortcuts” makes ctrl+= / ctrl+- make more sense in my mind, indeed.
They are probably going to be better than some sort of non-automatic (user-initiated) global vs local system. But yeah it's a whole separate thing to discuss.
somebody had that thought
Huh! I disagree with their choice lol but fair enough!
fine with me
💛
would be nice if we could print a warning
Ohhh, something like printing to stderr after parsing the keybinds. Yeah I can do that 👍 Sounds good.
I like to live by “solve the problem once you have it”, so waiting for someone to bring up a need to rebind color hotkeys does sound reasonable to me.
look more polished
I agree, yeah. In my personal fork where I use the feature, I set toolbars to hide by default to avoid seeing them not update, lol. So I'd like to solve this as well.
If it still doesn't work then I don't think it's a deal breaker
That's reassuring, thank you. I'll try to figure this out, and if I can't, I'll try my best to concisely write out what I learned / discovered, so that we could go fom there.
might cause a merge conflict then
Good to know. I'll rebase and solve it if it comes to that.
About merging btw: I think it's a really good idea to merge + squash, rather than just merge, when you merge prs. So that satty's history isn't riddled with internal pr-branch “address feedback” commits and such. Will help satty be buildable at any commit, too, for git bisect purposes.
About merging btw: I think it's a really good idea to merge + squash, rather than just merge, when you merge prs. So that satty's history isn't riddled with internal pr-branch “address feedback” commits and such. Will help satty be buildable at any commit, too, for
git bisectpurposes.
Yeah, we kinda had that discussion a while ago. My opinion was that authors should decide how many visible commits they want to split something into, after all they know how much credit they'd like for their work and they could always rebase, squash etc themselves while developing. But in the meantime I realised that PR authors rely on maintainers to decide that, so there's that.
Having buildable commits for bisect is definitely a good point. I do vaguely remember to commit stuff only when it compiles, I'd kinda hope that at least all commits that are pushed follow that rule -- but perhaps that is ancient knowledge. :)
Maybe we'll reconsider this, although that would mean that new PR authors have an uphill battle regarding the contributor stats. :wink:
rely on maintainers to decide that
Yeah, precisely. I'm used to projects squashing my commits for me, so I was a bit concerned to see my half-heartedly named “fix thing”-type commit on the main branch; unsquashed and in all its naked glory (lol)
hope that at least all commits that are pushed
That's the caveat: you make a commit, and after the fact realize that it doesn't actually build. So you make another commit where you fix it.
You push both of the commits.
Now later on the line, when someone is trying to bisect, they happen across the commit that doesn't build, rather than the exact next one that does.
uphill battle regarding the contributor stats
My dear dopamine green squares! Lol :D
There's something cool about being a stoic contributor, though 😎
For now I'll take my committing slightly more seriously / squash more often.
Well, that was easier than expected!
Now the ui updates when you pick colors with hotkeys 🥳
However, it's slightly yucky. Not sure if significantly so.
In the latest diff you can see me adding a “please update the ui” line; but if you picked a color with the mouse, it would already be updated.
So essentially I'm updating the ui twice.
In usage, it works regardless, and I don't imagine this is of any performance concern.
But as a programmer, it feels a bit yucky to do; I don't seem to have access to the correct object in the place where I'm supposed to be adding this line. I'll have a look around.
This is the place where I realistically should be doing the ui update.
I looked through Controller<StyleToolbar> and realized that indeed, I don't have access to StyleToolbar directly — so I can't update it there.
Updating the widget here is a lot more reasonable, but once again it bothers me how when the user uses the mouse to select the color, this change_state is technically useless.
The with-mouse updating happens somewhere here, with I can only assume magic 🧙
The change_state function isn't defined in the satty codebase (ripgrepped for it) — only used, so I think being okay with this double update is the way to go in this case.
A bit yucky, but I'm not seeing how to unyuck it, hahaha
This is the place where I realistically should be doing the ui update. I looked through
Controller<StyleToolbar>and realized that indeed, I don't have access toStyleToolbardirectly — so I can't update it there.Updating the widget here is a lot more reasonable, but once again it bothers me how when the user uses the mouse to select the color, this
change_stateis technically useless.The with-mouse updating happens somewhere here, with I can only assume magic 🧙 The
change_statefunction isn't defined in the satty codebase (ripgrepped for it) — only used, so I think being okay with this double update is the way to go in this case.A bit yucky, but I'm not seeing how to unyuck it, hahaha
I'll have a closer look when actually reviewing this, but I think that's exactly how the code place I linked above works. So should be fine to do the same thing.
yay :3 thanks for making it easy to contribute! you navigated me really well, and made really thoughtful considerations !