-
Notifications
You must be signed in to change notification settings - Fork 272
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
New canvas cursor and quick cursor implementation #1806
Conversation
As it currently is, it does not make sense to keep it as a cursor modification as it has no relation to the cursor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found two small issues in this PR:
- After disabling the dotted cursor in the preferences, it continues to linger on the canvas. When the program is started with dotted cursor disabled, the quick sizing visualisation lingers after using quick sizing. (also, now that i think about it, maybe we should rename the dotted cursor setting now that the cursor is no longer dotted. maybe “preview stroke width and feather” or something like that)
- Canvas cursor doesn’t seem to work correctly for vector bucket tool, though quick sizing works fine
In my experience, this PR also noticeably improves quick sizing performance, so I’m marking it as closing #1694. Well done!
PR is ready to be reviewed again.
This should be fixed now, although it required a bit more logic but overall it's better I think. The quick sizing visualization does not linger anymore either.
This should be fixed now
I've made a proposal for that for now, "Canvas cursor" is fine i think.
Hmm I'm in doubt about this one... I don't think it should have a cursor, because it doesn't draw on the canvas with a width size as such, we use it as a stroke tool to allow filling across multiple areas but the width does not matter. Either I should add the cursor again for the vector layer or we should remove the quick sizing property for this tool. Does it even make sense to have it? Also thanks for reviewing 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good now. I took the liberty of changing the config file key back since it’s not directly user-facing but messes with users’ settings when switching between versions. I’m fine with “Canvas Cursor” as the name in the preferences dialog and the code constants.
the width does not matter
But it does? Width = 1:
Width = 100:
That said you’re right that canvas cursor / quick sizing usefulness is limited here. I’m fine with either solution.
Lastly, I did notice a strange glitch when leaving and re-entering the canvas with the cursor after having made at least one stroke:
Screencast_20240305_214239.webm
(Edit: I just realised the recording also shows the cursor icon being wrong outside the ScribbleArea, but that is a bug in the Plasma 6 RC I'm using as far as I can tell and not the glitch I’m talking about)
Doesn’t seem to have any serious consequences though so I don’t think it’s strictly necessary to fix it, but I wanted to mention it at least.
Anyway, thanks for your work on this 👍
Co-authored-by: Jakob <[email protected]>
Fair enough, although i think the change is somewhat valid, even if it isn't directly user facing, it does make more sense that our keys are aligned with implementation or generic enough that we don't have to change it between versions. You do however have a point in that if you come back to the old version, then the setting should still be used.
Hmm it's not something I can reproduce on Mac OS, I don't see how that interpolation should happen either, so maybe it's a platform specific thing?
Correct but i don't consider that a "stroke" as such, I personally don't think it should be on the bucket tool thing at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how that interpolation should happen either
I just tested again and it is in fact interpolation. I didn’t figure that out at first because you have to make a stroke after changing the interpolation setting for it to take effect, and that’s probably also why you weren’t able to reproduce it. That said, I also noticed it happens on master, too – I just never noticed it because due to the way the canvas cursor is drawn on master it doesn’t stand out as much. So, sorry for the false alarm!
With that, I’d say this PR is good to go as soon as the vector bucket tool is taken care of.
I just realised there was an error in my tool event handling suggestion as I’d gotten some stuff mixed up while reading through the Qt documentation. I’ve just committed what I now believe to be the intended way to handle such things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Here's a proposal for a new canvas and quick cursor implementation.
The PR also introduces an arguably better way to interrupt a pointer events without potentially screwing up all our tools. We do this through a new method called interruptPointerEvent in BaseTool and currently use it in StrokeTool.
Screen.Recording.2024-01-09.at.19.45.33.mp4
Canvas cursor
The design of the canvas cursor has been tweaked, mostly in order to avoid cosmetic misalignment once the cursor becomes too small.
Quick cursor
When enabling quick cursor using the shortcut, the canvas cursor will be offset so that the respective circle outline aligns with the cursor position. if you adjust the width it will offset based on the outer circle and otherwise the inner circle
I initially wanted to just fix the Quick cursor implementation but ended up re-doing the entire canvas cursor implementation. The rework allowed me to also move the canvas cursor logic out of ScribbleArea.
Bugs fixed:
Changes: