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

Tweaks to comment presentation, hyperlinks in lyrics #846

Merged
merged 3 commits into from Dec 18, 2022

Conversation

mw9
Copy link
Contributor

@mw9 mw9 commented Dec 16, 2022

These proposed changes add a few "tweaks" to the treatment and presentation of trackinfo comments, and extend the ability to follow hyperlinks to trackinfo lyrics (where these are derived from a music file tag).

I think that most users wouldn't notice the changes. I think they make small, marginal, improvements, but that's a matter of taste/personal preference.

  • Add link support for lyrics in Default & Classic skins

Adds hyperlink support to the rendering of lyrics in the web interface. Some publishers do include hyperlinks in their lyrics tags, and it would be good to be able to follow these. It uses the same scheme as recently implemented in respect of hyperlinks in comments.

  • Treat multiple comments as multi-line, i.e. join with "\n" in place of "/"

Some file formats allow for inclusion of multiple comment tags. LMS joins these into one composite comment text, by concatenating with a " / " separator. While this works reasonably well with short comments, I find that longer comments become difficult to read.

This change has the comments joined with a newline, turning then into multi-line comments and, in my view, easier to follow.

  • Tweak display of multi-line and lyrics info items in web interface

This is a very small change to the Default & Classic skins. Its intention is to keep text in longer comments and lyrics grouped together a little better than they are now, and make them easier to read. It would not impact shorter comments adversely.

This change adds hyperlink support to the rendering of lyrics in the
web interface. Some publishers do include hyperlinks in their lyrics
tags.

It is a follow up to commit #4124b3,
"Improve/re-introduce rendering of links in comments etc"
…f "/"

This change modifies the way multiple comments are combined together.

Some tagging schemes allow multiple comments. LMS expects a single
comment, so they need to be combined.

With this change, comments are combined with a newline separator, in
place of the existing " / ". They then effectively become multi-line comments.

The change also eliminates the removal of an 'eng' prefix to a comment. It has
been in the code since at least 2005, and appear to serve no purpose. It may
have once been needed to remove an 'eng' language tag that is found in
ID3 comments. But these tags are already removed by LMS during the scanning
process.
@mw9 mw9 changed the title Feature/more comments Tweaks to comment presentation, hyperlinks in lyrics Dec 16, 2022
@@ -377,6 +377,10 @@
IF !title.defined || title == ''; title = item.title; END;
title = title | html | html_line_break;
IF item.parseURLs; title = title FILTER parseURIs; END;
IF item.wrap;
# Put wrapped text into a single element. Improves display of multiline comments & lyrics.
title = "<span style = \"display: inline-table;\"> $title </span>";
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... can't find inline-table, but only inline-block? (https://developer.mozilla.org/en-US/docs/Web/CSS/display?retiredLocale=de) .

And don't you print the title twice in this case?

And I actually don't understand how this helps with multi-line comments.

Copy link
Member

Choose a reason for hiding this comment

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

Would white-space: pre-line; be what you need here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inline-table appears about half way down that page you linked to. Having said that, I will have another go. I can't properly remember why I found inline-block lacking, now. And I'll look at white-space: pre-line.

title is only printed once. The $title that you see is being interpolated into a new string value for title.

Copy link
Contributor Author

@mw9 mw9 Dec 16, 2022

Choose a reason for hiding this comment

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

The difference I found between inline-table and inline-block is illustrated in the images below. As you will see, with inline-table the top of the display box aligns with the "Lyrics:" label, and it looks right. But with inline-block it's the bottom of the display box that aligns with the "Lyrics:" label. That doesn't look right at all !

Perhaps there's a 'proper' way to do this. I know too little about HTML to know where to look.

Screenshot 2022-12-16 at 20 24 24
Screenshot 2022-12-16 at 20 22 58

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I See! I didn't get the layout idea. But this makes sense. Now what I don't like is to have the actual style mixed in with the structure. I'd prefer it if you could eg. define an additional class .wrappedMenuItem, then defined

.browsedbListItem .wrappedMenuItem {
   display: inline-table;
}

(or whatever is needed to make this work)

That would allow every skin to handle this the way it wants. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you have an example file you test this with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a fiddle with white-space: pre-line;. In principle it is exactly what is wanted, except that the preceding html_line_break filter already "replaces" newline characters with a <br>. By "replaces" I think I mean "introduces", because the generated html still seems to contain the newline characters. So white-space: pre-line; would end up double counting line breaks. That took a while to notice, and figure out !

I've moved the style from the templates to the three slimserver.css style sheets, as suggested. The EN is just a placeholder, Classic is as expected, and Default adds an additional helpful indentation.

I will try and assemble a sensible example file or two and post them over the next 24 hours or so.

This change adjusts the display of comment and lyrics text in the
Default & Classic web interfaces. Its intention is to make longer
texts more readable by keeping the content together in a block.

LMS tags both comment and lyrics info items with the 'wrap' attribute, and
this is how they are identified.

LMS does not tag any other info items in this way. But if it, or a plugin,
does, then this slight change in behaviour will also apply to them.
@mherger mherger merged commit 6ed4016 into LMS-Community:public/8.4 Dec 18, 2022
@mherger
Copy link
Contributor

mherger commented Dec 18, 2022

Thanks a lot!

@michaelherger
Copy link
Member

BTW: I removed (together with some others) the placeholder from EN/slimserver.css again as they're no-ops.

I actually could have moved the inline-table to that file instead, and removed the others... IIRC they would inherit it. Oh well...

@mw9 mw9 deleted the feature/more_comments branch December 18, 2022 17:43
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

3 participants