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

Consistent text rendering for different pattern types and exposure of the background color to style sheets #3704

Merged
merged 4 commits into from
Jul 20, 2017

Conversation

michaelgregorius
Copy link
Contributor

Render the text labels of the different types of pattern consistently:
3674-consistent-patterns

Do not render any label in case the trimmed text becomes empty, i.e. if the user enters a single space (" ") for the name this would not be rendered.

Expose the background color of the pattern text label to the style sheets.

@PhysSong
Copy link
Member

Looks fine. Let's wait for @Umcaruje.

@@ -620,15 +620,17 @@ TrackContentObjectView {
qproperty-selectedColor: #006B65;
qproperty-BBPatternBackground: #373d48;
qproperty-textColor: #fff;
qproperty-textBackgroundColor: rgba(0, 0, 0, 75);
qproperty-textShadowColor: rgb(0,0,0,200);
Copy link
Member

Choose a reason for hiding this comment

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

Now that we actually have a shaded background, is there even a need for a shadow for the text?

Copy link
Member

Choose a reason for hiding this comment

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

Now that we actually have a shaded background, is there even a need for a shadow for the text?

Should be tested with custom colors.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tresf The CSS now gives great flexibility with regards to the colors. So if the user decided to make the patterns white it would be wise to take a different color for the text background or the text color itself. I also don't think that the current text shadow implementation works well with HDPI displays in some of the examples above because it would only give a line that is one pixel wide.

@Umcaruje If we decide to drop the text shadows then I guess we could also remove the qproperty-textShadowColor property.

Copy link
Member

@tresf tresf Jul 18, 2017

Choose a reason for hiding this comment

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

@tresf The CSS now gives great flexibility with regards to the colors. So if the user decided to make the patterns white it would be wise to take a different color for the text background or the text color itself

I'm talking about testing the built-in functionality. This has nothing to do with custom themes or custom CSS, this has to do with testing this feature against the color capabilities of our BB patterns. (as depicted above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tresf Ah, ok. It looks like this:

3704-colored-bb-patterns

Copy link
Member

Choose a reason for hiding this comment

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

@michaelgregorius that looks fantabulous.

Copy link
Member

Choose a reason for hiding this comment

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

I still kinda like it with the shadow now that I look at it, makes it more readable imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Umcaruje Commit 8228936 adds the ability to render shadows back. The rationale is that if a design does not want to use shadows it can just set the property qproperty-textShadowColor to something completely transparent. That way we are more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With shadows:

3704-colored-bb-patterns-shadowed

For my taste it looks cleaner without the shadow. It might also make sense to investigate whether it would make sense to use QGraphicsDropShadowEffect to render the shadow. But that would be something for another pull request.

@Umcaruje
Copy link
Member

This looks good. The only problem I see is that the eliding code now won't show text when a pattern is very small. Example:
gifrecord_2017-07-18_151425

@michaelgregorius
Copy link
Contributor Author

michaelgregorius commented Jul 18, 2017

@Umcaruje What do you think about the following solution? As long as the elided text is longer than one character it is used. If the text is shorter than two characters, i.e. it only consists of "..." or becomes completely empty due to the elision, we just render the trimmed text so that something is drawn at all:

lmms-elide-example

Edit: "anything" -> "something"

@tresf
Copy link
Member

tresf commented Jul 18, 2017

I don't mind the cut-off behavior of the pattern name now. Perhaps we change the tooltip to display the full name instead of the pointless "double-click" hint. As @budislav has previously mentioned, we shouldn't be hiding one-time-use help text inside our UI. :)

@Umcaruje
Copy link
Member

@michaelgregorius I like your cut-off proposal, and I think eliding does add a bit of class and consistency, but @tresf makes a very good point on utilising the tooltip, but I think that could be done in another PR.

@tresf
Copy link
Member

tresf commented Jul 18, 2017

and I think eliding does add a bit of class and consistency,

Agreed. It's more consistent with the rest of the software for sure. I use quite a few of these smaller patterns in my productions, so I'll have a bunch of S... named patterns which makes me lean in the cut-off direction, but both implementations have merit.

@michaelgregorius
Copy link
Contributor Author

So is everybody OK with merging this one? It provides new options and with the current implementation it's even possible to completely reinstate the old style.

Add the method paintTextLabel to TrackContentObjectView. This methods
implements the painting of a given text on a given painter. The new
implementation does not draw any text label in case the trimmed text
becomes empty.

Use the method paintTextLabel to paint the text for automation patterns,
BB patterns and instrument patterns.

Adjust the style sheet of the classic and default theme by moving the
font definition from PatternView into TrackContentObjectView so that it
can be used by all inheriting classes.
Add a new property "textBackgroundColor" to TrackContentObjectView to
expose the property to the style sheets. Use this property in the code
that renders the text labels.

Adjust the two existing style sheets.
Render the whole text if the elided text becomes empty or if it only
contains one character, i.e. if it becomes "…". Solved as implemented
because I was not able to check for "…" explicitly, i.e. the comparison
against "…" still failed.
Add back the rendering of text shadows for pattern labels. If some
design does not want shadows beneath the text it's always possible to
set the CSS property qproperty-textShadowColor to something completely
transparent.
@michaelgregorius michaelgregorius merged commit 17f1856 into LMMS:master Jul 20, 2017
@michaelgregorius michaelgregorius deleted the 3674-FixTCORendering branch July 20, 2017 17:53
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.

None yet

4 participants