Add more customization to label

[abisammy]

This adds more customisation to the label.

Firstly it changes the dropdowns to a single string with substitutions:

This can then be changed (for example):

Which is reflected on the label:

This allows for much more customisation and layout of the label

It adds a description for the substitutions:

As well as simplifies adding more fields, for example the app it is playing on and so on

[abisammy]

Some more examples:

[Batwam]

Hi,

I would recommend including the info/description as a tooltip for consistency with the test if the preferences. This should simplify the code too as you should be able to use the regular addEntry (see example in the PR below too).

With regards to specifying inputs which are then used as regex, you might need to ringfence/filter the implementation. See this discussion for details: https://github.com/Moon-0xff/gnome-mpris-label/pull/56#issuecomment-1560469956

[abisammy]

Hi,

I've just commited a fix for making the format options into a tooltip, as seen below:

As for the regex, the I'm not too sure what you mean by ringfencing? The input string is not used a as regex rather used as an input for a replace using a regex?

[Batwam]

Yeah, looking at it again, I think that you are right about bad regex not being a concern as with your current implementation, you are applying the regex changes to the user's entry rather than the other way around.

[Moon-0xff]

Hello @abisammy, thanks for the PR, it looks like you've been able to follow our stuff pretty well.
I've been thinking of adding something like this to the extension from some time now, so this change is quite welcomed.

However I think approaching the problem with a regex might be over-complicating things?

String formatting sounds like the kind of problem JavaScript should have a built-in way of doing effortlessly.
By searching on the documentation a bit I've found this page about template literals that might be a better solution.

[Moon-0xff]

I should also point out that versions are assigned by ego. When adding future changes to the changelog we add them as Upcoming Version, and keep the metadata unchanged. Like on this commit.

[abisammy]

Hi!

To my knowledge I believe that is not posssible with template literals. Although they can be used to embed variables in a string inside the code, they cannot be used for user inputs in javascript. For example inside the code, template literals would render the string `Hello ${name}` replacing the variable, however if you have a string as an input if you use template literals the strings value would be Hello ${name}.

Therefore I believe regex would be the only suitable method to perform this!

[Moon-0xff]

I don't know if template literals are capable of doing this, but JavaScript not having a more straightforward solution for this sounds hard to believe!

If that's the case then i would want to refactor it in a less scary way, the code is a bit dense and I foresee some users, even not the programmer type wanting to modify label.js for some reason or another.

[abisammy]

Just commented the code, please let me know if this is less scary

[Moon-0xff]

Still scary 😨
But the comments help! Don't worry about it, I'm definitely asking for something too abstract and opinion-based, I will try to change it later.

For now, I'm more concerned about the possibility of user input messing up things, judging by the code alone it looks like that isn't a possibility, but we should test if inputting something that an user might enter doesn't breaks things regardless.

[abisammy]

Works with empty strings, and strings with no substituions.

A problem I can see is a user inputting a really long string (like 1 million characters). This can be fixed by limiting Gtk input though. What would you recomend for maximum length, 200 characters?

[Moon-0xff]

I'm pretty sure in that scenario Gtk will just cut off the excess and add ... it already does that if the label exceeds the designated space in the top panel.
Have you tested it?

[abisammy]

Yep been tested and works!

[Moon-0xff]

Ok, there's some small changes I will add before merging to main, there's also a few things to test.
I will push directly to the branch if you don't mind.
Do you mind?

[abisammy]

Nope! Feel free

[Moon-0xff]

As showed in the commits I refactored the substitutions code completely.

The refactor shares these problems with the original code:

• LabelString.replace only replaces the first ocurrence of key, this is expected
• Characters between fields are preserved even if the fields are empty, illustrated: Example Title | | Example Artist

There's also the check against no substitutions, which isn't implemented on the refactor.

[Batwam]

Replacing all found instances is achieved with a regular expression and global flag as described here: "this is a string".replace(/ /g, "-")

[Moon-0xff]

replaceAll is probably a better solution?

I was thinking of leaving it like that but if replaceAll works then it's an easy fix.

[Moon-0xff]

works!

[Batwam]

Nice. You might also want to update the tooltip to add the 2 new substitution options?

[Moon-0xff]

Yes!

[Moon-0xff]

The second problem I've mentioned might be tough to solve.

I'm thinking of adding a mechanism to bundle characters to a substitution, this isn't a simple solution at all.
Here's the syntax I'm thinking of:
{%ARTIST% | }{%ALBUM%}{ | %TITLE%}

I will not push more commits in at least 12 hours so feel free to push!

[abisammy]

Looks good! although there are a couple changes I would want to make.

1. You removed some of the code that makes checks for empty substituions, therefore if there is no data the empty string is rendered: This was replicated from the previous system, that the string wouldn't render if there were no substituions. However it can be lfet out and then we can use the system you mentioned above
2. I believe a max length is necessary as it prevents the user from making the input string too long. This can be set to quite high, but wouuld prevent them from inputting too many characters

[abisammy]

The second problem I've mentioned might be tough to solve.

I'm thinking of adding a mechanism to bundle characters to a substitution, this isn't a simple solution at all. Here's the syntax I'm thinking of: {%ARTIST% | }{%ALBUM%}{ | %TITLE%}

I will not push more commits in at least 12 hours so feel free to push!

What do you mean by this, do you mean render another text if the previous value is not avaiable? If so, I think this can be done with regex

Edit: Nevermind, just saw the previous post. I don't see why this isn't possible, will work on a solution later.

[abisammy]

I'm thinking of adding a mechanism to bundle characters to a substitution, this isn't a simple solution at all. Here's the syntax I'm thinking of: {%ARTIST% | }{%ALBUM%}{ | %TITLE%}

This has now been implemented. This is the format: {{This is my string with VALUE}}{{ARTIST|ALBUM}}

The first set contains the brackets. Curly brackets can also be added by using \{ \} in the first bracket. It will always be VALUE, which will be replaced with the key in the second set of brackets.

removedEMPTY is new special field, which will render an empty string if no data is present. Values are sepearated by |, and will take priority from left to right. They don't use the %, so the replace all from the old system (which is still implemented) replaces these values.

The system is fairly robust, please let me know if you have any questions!

[abisammy]

This still needs to be documented. I think it should be added to the tool tip, and we should also include a link to a paragraph in the README, due to the more complicated syntax. We would also then be able to include more examples.

[Moon-0xff]

Can you replicate the default label with this system?

[abisammy]

It would be: %ARTIST%{{ | }}{{ALBUM|EMPTY}}%ALBUM%{{ | }}{{TITLE|EMPTY}}%TITLE%

New notation: %ARTIST%{{ | }}{{ALBUM}}%ALBUM%{{ | }}{{TITLE}}%TITLE% Alternativley: %ARTIST%{{ | VALUE}}{{ALBUM}}{{ | VALUE}}{{TITLE}}

[abisammy]

I think empty should be there by default, since you will only use this notation to hide elements if there is no data, therefore I'm going to modify it a bit.

[Moon-0xff]

Wow, i didn't expect to be possible because the values depend of the previous/next block.

Before reading your recent comments I was writing about maybe adding more syntax, something similar to look ahead/behind in regex.

It looks like isn't necessary, but it might be a good addition as the current syntax is kind of confusing.

[abisammy]

I also just added a new way of formatting it. I belive a look behind would add uneccessary code complexity, and this format is simpler anyway: %ARTIST%{{ | VALUE}}{{ALBUM}}{{ | VALUE}}{{TITLE}}

Apart from this, are there any more features that should be added or does it just need to be tested now?

[Moon-0xff]

Feature wise I think it's complete.

But I'm not convinced with the syntax, certainly it will be tough to explain it.
On the other side it might be the best compromise between understandable code and understandable syntax.

I definitely need to examine the code more closely.

[abisammy]

The regex can be updated for other notations, for example this can be used: {{my string,values}} to replicate an array.

That would lead to a default value of: %ARTIST%{{ | VALUE,ALBUM}}{{ | VALUE,TITLE}}

More notations could be: [my string,values] - %ARTIST%[ | VALUE,ALBUM][ | VALUE,TITLE] or (my string, values) - %ARTIST%( | VALUE,ALBUM)( | VALUE,TITLE)

[abisammy]

Another idea I want to implement is more substitutes in the new notation such as:

• Any - This will render the string if any of the substitutes declared in the string are invalid. This can be used to not render text if no substitutes are used throughout the whole string
• Empty - This will render the string even if no substitutes are made

[abisammy]

Another idea I want to implement is more substitutes in the new notation such as:

• Any - This will render the string if any of the substitutes declared in the string are invalid. This can be used to not render text if no substitutes are used throughout the whole string
• Empty - This will render the string even if no substitutes are made

This has now been implemented. Please let me know what you think of the new notations: https://github.com/Moon-0xff/gnome-mpris-label/pull/58#issuecomment-1570668122

[Batwam]

The thing with this sort of custom syntax is that it quickly becomes only usable by whoever came up with it. I would keep it simple and have a syntax which is as obvious as possible.

I kind of liked {%ARTIST% | }{%ALBUM%}{ | %TITLE%} but that's probably as complex as I would make it. the reality is that I'm not sure it will be possible to fix all edge cases. In nearly 100% of cases, there will be a %TITLE so, default should probably be {%ARTIST% | }{%ALBUM% | }{%TITLE%}. I wouldn't use ( as this could be used if someone wanted to have something like %TITLE%{ (%ARTIST)}

[abisammy]

The thing with this sort of custom syntax is that it quickly only usable by whoever came up with it. I would keep it simple and have a syntax which is as obvious as possible.

I kind of liked {%ARTIST% | }{%ALBUM%}{ | %TITLE%} but the reality is that I'm not sure it will be possible to fix all edge cases. In nearly 100% of cases, there will be a %TITLE so, default should probably be {%ARTIST%}{%ALBUM% | }{%TITLE%}. I wouldn't use ( as this could be used if someone wanted to have something like %TITLE%{ (%ARTIST)}

I understand your feedback, but I think that this: %ARTIST%{ - VALUE,ALBUM}{ - VALUE,TITLE} has the advantage, that if the value is empty (for example there is no album, then something else can be rendered, for example: { this string with VALUE,ALBUM|TITILE} will render the title there if the album is empty

[Moon-0xff]

I can't really decide what needs to be changed, or what step to take now. I need to think hard about this one.

For the meantime we should focus on the other PRs.

[Moon-0xff]

Ok, I thought about this PR a lot and I think it's better to not merge it. While it looks like it provides more flexibility than the current approach, ultimately it doesn't.

• A simpler implementation can't match the features we already provide
• A more complex implementation could match and even surpass the features we provide, but requires less intuitive syntax
• Striking a balance between flexibility and intuitive syntax can be achieved by using (for example) lisp syntax, but that only caters to the programmer-type
• The programmer-type user is better off editing the source code directly

I think our current approach of shaping the format with entries and other Gtk widgets is pretty good at allowing flexible customization while not demanding much of users. And we could improve it!

56 it's a great change to add, I'm also thinking of improving on the handling of the character limit (max-string-length).

Those changes depend in the current fixed format of the label, but I think it's completely possible to free the format a bit without losing features (and potential features). Not sure how though.

[Moon-0xff]

I will however, leave this PR open. And I'm open (and asking!) for second opinions.

I think this feature could be added and make perfect sense with some tweaks, or if we approach it from another angle.
I was thinking of adding it with a fallback to the fixed format, however it seemed redundant. I'm sure we can free the format. right?

[abisammy]

I will however, leave this PR open. And I'm open (and asking!) for second opinions.

I think this feature could be added and make perfect sense with some tweaks, or if we approach it from another angle. I was thinking of adding it with a fallback to the fixed format, however it seemed redundant. I'm sure we can free the format. ~right?~

I think we can add the old system in a slightly different way.

We add the dropdowns, and add a toggle for use advanced formatting. If the toggle is on we show the textbox, otherwise show the dropdowns Whenever the dropdowns update, instead of having to rely on two systems at once, we make the dropdowns update the string. This reduces redundency

[abisammy]

Can someone please review the merge, making sure that all files were merged OK?

[Batwam]

What is the final agreement regarding whether this feature should/shouldn't be merged?

Merge-wise, the label filtering introduced a few days ago seems to be removed here. Even if the method changes, we should still be able to filter %TITLE%. Could you double check if there is some incompatibility in the codes.

[Moon-0xff]

@Batwam see above for my reasons to not merge this ( I will not close it either).

[Moon-0xff]

Making a patch out of this PR

As mentioned I don't want to close this PR, the changes are problematic but overall a good addition to the extension.

However maintaining a diverging branch is an avoidable work. We could simplify this PR and create a patch out of it. It will require maintenance too but far less.

[abisammy]

There seems to be alot of changes planned, once done I will make a new branch and commit the changes...

[Moon-0xff]

Closing this PR now.

As mentioned, this feature could be added as a patch.