Closed crumblingstatue closed 1 year ago
Unresolved questions:
Patch coverage has no change and project coverage change: +4.47
:tada:
Comparison is base (
6a4f2ba
) 20.74% compared to head (4ce2752
) 25.22%.:exclamation: Current head 4ce2752 differs from pull request most recent head cd2acc7. Consider uploading reports for the commit cd2acc7 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I like it! For the color, maybe a lighter version of the normal color? Could also be a colored highlight. Feel free to choose.
Please pass LapixState
in the sync
phase, so we can have access to selected_tool
in self
. Before calling update we usually call sync
on gui components, passing any updated info they need. On update they read from their own state. This is explained in more detail here: https://github.com/yds12/tarsila/blob/master/ARCHITECTURE.md#frontend-lifecycle
Other than that PR looks good to me.
Update: sorry, I made a confusion above, we need to pass just the selected tool in the sync
phase, not the whole LapixState
(that would be impossible due to lifetimes etc).
Thank you, I addressed your responses. This is ready for review now.
Hmm, actually there is one notable issue. I'm currently using Ui::scope for styling, but it seems to destroy the wrapping layout of the toolbar buttons.
Before:
After:
I'll try to look for some solution.
I'm working around the Ui::scoped
problem by manually resetting the old style.
Let me know if that's an acceptable solution to you.
Thank you! It's a subtle but nice addition. The workaround is ok, if we find another way later we can try to fix but it's not a big deal.
Merging!
This is a proof-of-concept feature of highlighting the active tool. This allows the user to see what the current tool is, even if they are using the crosshair cursor.
In this screenshot, the eyedropper tool is active.