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

Status color of glyphs #1342

Closed
wants to merge 20 commits into from
Closed

Status color of glyphs #1342

wants to merge 20 commits into from

Conversation

ollimeier
Copy link
Collaborator

Fixes #926

Here a screenshot:
Screenshot 2024-05-07 at 11 44 28

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! Looks very good.

Possible refinements:

  • reduce the opacity of the color while in edit mode, maybe 50%?
  • while I don't think the thickness of the color bar should be fully fixed in screen pixels, I feel it is getting too thin at very small sizes, and too dominant when zooming in. Suggestion: define a minimum and maximum thickness in screen pixels, and clamp the 50 value between them. Something like this:
  const thickness = clamp(50, parameters.minThickness, parameters.maxThickness);

Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

  • Ane minor suggestion
  • The value of "50" should also be related to the units per em (but see also previous review)

@ollimeier
Copy link
Collaborator Author

Thanks a lot for your feedback, Just.
I added a opacity for the status color. But to make it work I had to create a new function rgbaToFillStyle, because rgbaToCSS does not work with the alpha value for canvas 2d, please see the following documentation:
https://doc.qt.io/qt-5/qml-qtquick-context2d.html#fillStyle-prop
Or do you have a different idea how to solve this?

Also, to make it work with two different colors (with and without transparency) for editing and non-editing mode had to split it into two separate registerVisualizationLayerDefinition or is there a way to make it possible with one, which I am not aware of?

Beside that, I tried to implement const thickness = clamp(50, parameters.minThickness, parameters.maxThickness);, but when I added minThickness and maxThickness to the parameters, the values were scaled (related to the zoom), resulting in weird values/sizes for the status color box in the background.

I hope this all make somehow sense to you.
Here a short demo video of the modifications:

fontra-status-color.mp4

…ed-glyph

Add isSelected and isEditing properties to positionedGlyph
Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

I added a opacity for the status color. But to make it work I had to create a new function rgbaToFillStyle, because rgbaToCSS does not work with the alpha value for canvas 2d, please see the following documentation:
https://doc.qt.io/qt-5/qml-qtquick-context2d.html#fillStyle-prop

It is not that rgbaToCSS() does not work with the alpha value for canvas 2d, it does not work with the alpha value, period. You have discovered a bug :) So instead of adding a new function, you should fix the bug in the existing function.

Also, to make it work with two different colors (with and without transparency) for editing and non-editing mode had to split it into two separate registerVisualizationLayerDefinition or is there a way to make it possible with one, which I am not aware of?

Good point. While it is possible to compare positionedGlyph to model.getSelectedPositionedGlyph(), this is not ideal, and I have created #1345 to make this easier. Rebase to main, and you can use the positionedGlyph.isEditing flag.

Beside that, I tried to implement const thickness = clamp(50, parameters.minThickness, parameters.maxThickness);, but when I added minThickness and maxThickness to the parameters, the values were scaled (related to the zoom), resulting in weird values/sizes for the status color box in the background.

Having them scaled was exactly my intention. I want min and max values that are in pixels ("let the status bar not be thinner than X pixels and not be thicker than Y pixels"). So, calculate a base thickness by doing 0.05 * unitsPerEm (not scaled to pixels, this is in font units), and clamp that value between the scaled min and max values. Suggested minThickness: 3; suggested maxThickness: 15. We can fine tune later. This is exactly what I described earlier, though, but I hope it is clearer now.

src/fontra/views/editor/visualization-layer-definitions.js Outdated Show resolved Hide resolved
src/fontra/client/core/utils.js Outdated Show resolved Hide resolved
@ollimeier
Copy link
Collaborator Author

I am closing this PR, because there are some weird conflicts. I will create a new one in a sec.

@ollimeier ollimeier closed this May 9, 2024
Copy link
Collaborator

@justvanrossum justvanrossum left a comment

Choose a reason for hiding this comment

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

Some minor comments. Almost done!

@ollimeier ollimeier deleted the issue-926-status-colour-glyphs branch May 28, 2024 12:17
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.

Add function to show status colour of glyphs in canvas
2 participants