kaaholst / android-squeezer

Remote control for your Logitech Media Server ("Squeezeserver" etc) and players.
Apache License 2.0
75 stars 15 forks source link

Feature request: Display classical music tags #802

Closed schup011 closed 2 weeks ago

schup011 commented 8 months ago

I would like to ask whether it is possible to integrate some additional infos on the "now playing" that are common to classical music. In particular, I would love to see:

Would be perfect to spend 2-3 configurable lines for which you just specify the tags you want in the settings.

I could contribute myself, but I am not really familiar with the code and the tool chain. Therefore, I would need someone to give me some help to find relevant code sections. The info I am describing above is already available in the context menu (the three dots) aside of the album in the now playing screen.

Thank you very much for maintaining this great app!

kaaholst commented 8 months ago

Thanks for the suggestion.

First, please suggest where these infos would fit on the current now playing screen.

drawing

If possible also please attach how it currently looks on your device for a relevant song.

Regarding the code, this is defined in NowPlayingFragment.java. This class handles both the currently playing music at the bottom of the screen and the now playing page. The music info layout is defined in now_playing_include.xml which is used in now_playing_fragment_full.xml and now_playing_fragment_full_large_artwork.xml. The tricky part is that the available space is dependent on the screen size, form factor and orientation.

Song information is not directly requested from the server, as one might expect. Instead, we make a generic “More” query to LMS, and display the returned data. To request specific data is possible, but requires some additional work.

schup011 commented 8 months ago

Sorry for the late reply.

So my suggestion is

drawing drawing

(First screenshot is from the phone, second from a tablet).

So how to configure: When I press on the three dots beside the artist line, and then press "Further Infos" (the very bottom line), and then press "Show Tags" (the very bottom line), the following is displayed:

drawing

The tag names shown there could be used in the configuration, e.g. First line custom configuration string: "COMPOSER[:]" Second line custom configuration string: "TITLE" Third line custom configuration string: "ARTIST[ -] BAND [- ]CONDUCTOR" Fourth line custom configuration string: "ALBUM (DATE)"

(In my example, BAND and CONDUCTOR tags are not set). Of course, should be possible to choose the current "standard" configuration. Square brackets could be used to handle optional characters that are only inserted if the adjacent tag field is not empty.

(Sorry for the large pictures, I was not able to size them down).

schup011 commented 8 months ago

Song information is not directly requested from the server, as one might expect. Instead, we make a generic “More” query to LMS, and display the returned data. To request specific data is possible, but requires some additional work.

Thanks for the hint. Could you point me to the query of the "More" and/or to the tag information I have shown in my above post? I did not find it in the code...

kaaholst commented 8 months ago

Those are good ideas! So it's 4 configurable lines of song info on the now playing screen. This could be via up to 4 (maybe even 5 if it fits on some devices) configurable song-info lines via settings, where the defaults would look like the current setup. It will have to be considered where to put the context menu (3-dots), and the album-art if the large volume-control is selected. See below.

Regarding the image, I too thought if was a little big for this purpose. The GitHub Markdown doesn't directly support resizing images, but you can use an img tag instead, like this: <img src="https://github.com/kaaholst/android-squeezer/assets/428304/08821ee9-ff19-47a3-9ef2-8c3c7d003081" alt="drawing" width="200"/>

The “More” query is performed in the static ContextMenu.java.show method. This is called, for example, from NowPlayingFragment.onCreateView where the click handler for the context menu is set up. A word of warning, this is completely generic and not easy to follow. It is an implementation of SBS SqueezePlay interface. Basically, we fetch a player's home menu in SqueezeService.requestPlayerData and display the items we receive. For the now playing screen and the current playlist, we use the status command, but the format of the received tracks still follows the SBS SqueezePlay Interface. The status command is described in the LMS web interface, select Help / Technical information / The Logitech Media Server Command Line Interface (CLI). Since we don't know what the moreAction will return, we don't (programmatically) know how to execute the “More Info” or “View Tags” actions. We can't rely on parameters/texts from the responses, as this might change for LMS versions and language. For this reason and to keep things simple, I suggest to use a specific CLI command to fetch the track informations. I believe status - 1 will give the current track and the songinfo command defines which information is available.

I realize this is a mouthful, I'm happy to give it a shot to test things out if you like.

drawing
schup011 commented 8 months ago

Very good. Now I understand why I had such difficulties to understand the code sections that are retrieving song information. I also resized the images according to your suggestion. Looks better now. Thank you so much, so it would be great to move forward here. If you try something, I can easily test things on two different devices, and I can also support in coding once the difficult things (how to retrieve information etc.) are done (sorry).

schup011 commented 8 months ago

Just one "heads-up" from my side: if you completely change the way how you get song information, you will lose the "preformatting" that is done by LMS if you use "requestPlayerData". There are some LMS settings what should be displayed there. So I think even if I would be happy to see exactly what is defined by the song tags, many other users will probably like to stick to the the "preformatted" title and album information they are used to. So I would propose to keep a "legacy" mode that still uses requestPlayerData.

schup011 commented 8 months ago

It will have to be considered where to put the context menu (3-dots), and the album-art if the large volume-control is selected. See below.

I think that could remain as it is. The two lines just are a little shorter in this context.

kaaholst commented 8 months ago

I have created a branch SongInfo, which fetches the requested information, plus some more, from LMS. It is delivered to NowPlayingFragment via a MusicChanged event, as currently. MusicChanged.playerState.currentSong.songInfo contains the information you want to show. We subscribe to status changes from LMS. When we receive a change of the currently playing song, request the song information from LMS, and delay the MusicChanged event until song information is received. If the song information is not received within 100ms, the MusicChanged event is sent as is. You are welcome to take a look, and experiment with how it can look on the NowPlaying screen. I requested some more information from LMS, in particular I would also like to integrate bit-rate and sample-rate to ǸowPlaying. Ideally, the app should show the information which is present, and which fits the display! Maybe expand/collapse or scroll if it doesn't fit. (I know that might prove difficult)

schup011 commented 8 months ago

Thank you so much!

It will take a little time for me to get the environment running and to dig into it, but I certainly will. I will come back once I have made some progress.

kaaholst commented 4 months ago

Are you making any progress on this one? Let me know if you have any questions, or I can help in any way.

schup011 commented 4 months ago

I am really sorry.... I am still planning to start, but haven't really so far because many other things to do. I will try to come back within the next 6 weeks.

kaaholst commented 4 months ago

No worries, didn't mean to pressure you. Let me know if you have any questions.

schup011 commented 1 month ago

Hi, I finally got some first results. I was able to retrieve conductor, composer and band info, and I get them displayed in the existing lines (artistText, albumText). I will now try to create another line and to move the others a little bit to fit.

schup011 commented 1 month ago

Now I added two additional lines: One above TrackText and one other below ArtistText.

On my phone (6 inch), that looks good.

The only thing is that in landscape mode, the three (formerly two) lower text lines (AlbumText, ArtistText, and my new ConductorText) are too large in font size. Instead, in portrait mode the proportion is good (in my opinion). So I guess in landscape mode, those lines should better be made "Tertiary" in size instead of "Secondary".

And I had no success making one of those lines (ArtistText) also Marquee, thus that the longer line scrolls automatically. It can happen in classcial music that you got a longer list of artists. I tried to copy that from the TrackText style, including requestFocus(), but that didn't work.

I would suggest to make it switchable by preferences whether one wants today's setup with three lines or my new one with five lines. For the five line setup, we could start with a default setting for classical music, and then later add an option to customize content. What do you think?

kaaholst commented 1 month ago

Glad to hear you are making progress🙂 First I think you should merge (or rebase) the latest develop branch. There are a considerable amount of adjustments to the layouts, including font sizes. About when to include the new information, if we go for new settings, I would prefer to call them "include composer and conductor information" or similar, as it may be useful for other than classical music. You can also push your changes to your fork, then I can have a look at the ellipse thing. If it's experimental, you can create a temporary branch, which you can later remove if you like.

schup011 commented 1 month ago

OK, I will try to add a setting as suggested. I would then make the text field names more general, something like SongInfoText1, SongInfoText2, ... because that makes customization and variants more clear.

I forked from your SongInfo branch. Sorry to ask stupid questions (my first time to develop on GitHub), but my fork shows me:

This branch is not behind the upstream kaaholst/android-squeezer:SongInfo.

I guess if I should merge the latest develop branch, it would be needed first that you merge the changes from the develop branch into the SongInfo branch of your repo, right?

kaaholst commented 1 month ago

Well, you could fetch and merge from my develop branch. However, I merged the develop branch into SongInfo and pushed both to https://github.com/kaaholst/android-squeezer. So now you can fetch and rebase your SongInfo branch.

schup011 commented 1 month ago

Thanks!

One question for customizing the layout by settings:

I want to add two lines (one composer, one conductor) and want to turn them on by settings, either to have one of them or both. If I create a separate layout xml file for each variant, I end up in lots of different files with just few differences. Is it instead possible to configure TextViews such that if they are empty they disappear as if they had not been there, thus that the other lines can use their space? (Something like AutoSize with either normal size if there is content or zero size if there is none.) That would be much better because I could just post real content or empty content depending on the preferences.

schup011 commented 1 month ago

I am answering myself: I did that by:

if (!addComposerLine) { composerText.setVisibility(View.GONE); }

Seems to work really nice, you can do that dynamically and the layout is recalculated accordingly. But still have to test much more, and rebase my branch.

kaaholst commented 1 month ago

About where to put these settings, for consistency, I think it should be a “View” options menu like we have for the lists. See plugin_list_menu.xml. Or do have another idea?

schup011 commented 1 month ago

Yes, I did that, and it's working great. I will have to add some other things and to rebase, then I can commit a proposal to my repo.

22.05.2024 08:45:54 Kurt Aaholst @.***>:

About where to put these settings, for consistency, I think it should be a “View” options menu like we have for the lists. See plugin_list_menu.xml. Or do have another idea?

— Reply to this email directly, view it on GitHub[https://github.com/kaaholst/android-squeezer/issues/802#issuecomment-2123993428], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AON5TCY6DO53VEDK3GDPGBTZDQ5KDAVCNFSM6AAAAAA57ABZR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTHE4TGNBSHA]. You are receiving this because you authored the thread. [Verfolgungsbild][https://github.com/notifications/beacon/AON5TC77GYUQ6DGN6223C3DZDQ5KDA5CNFSM6AAAAAA57ABZR6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT6TGIVI.gif]

schup011 commented 1 month ago

I have never been working actively with git, so I am really stupid in handling these changes. I ended up in having created a new branch called master in my Repo that contains my commits. You can diff the branches SongInfo (which is identical to your branch, I rebased it) and master with my changes. Don't ask why I did not commit directly to my SongInfo branch...

I am really happy with the result. You now have three additional checkboxes in the view settings:

schup011 commented 1 month ago

One question (off-topic): If I choose a favourite or a track / album from a search list to play now, I find it a little strange that the view does not change to NowPlaying. Is there a reason for that? Where and how could that be changed? Is that in BaseActivity.java?

kaaholst commented 1 month ago

One question (off-topic): If I choose a favourite or a track / album from a search list to play now, I find it a little strange that the view does not change to NowPlaying. Is there a reason for that? Where and how could that be changed? Is that in BaseActivity.java?

This is because Squeezer has an always visible now playing fragment, either mini or full, which updates with the currently playing song, and is able to control playback. So it is a deliberate choice not to change to now playing. If you were to change this behaviour, I think it would be in JiveItemListActivity#nextWindow

kaaholst commented 1 month ago

I have never been working actively with git, so I am really stupid in handling these changes. I ended up in having created a new branch called master in my Repo that contains my commits. You can diff the branches SongInfo (which is identical to your branch, I rebased it) and master with my changes. Don't ask why I did not commit directly to my SongInfo branch...

I am really happy with the result. You now have three additional checkboxes in the view settings:

* Show composer: This adds an additional line above the track title that shows the composer with e.g. "Ludwig van Beethoven:"

* Show conductor: This adds an additional line below the artist line that shows the conductor and the album line is used instead for displaying band/orchestra (conductor does not make sense without an orchestra)

* Trackinfo for classical music: This adds descriptions to the artists like "Soloist: " or "Conductor: " and it removes empty artist lines, so it will not show "unknown artist", but remove the line from the layout.

No worries, git can be a mouthful! master is the name the default git branch name. In Squeezer we use it to reflect what is currently in production. The develop branch contains changes that will be in the next version, and it shall be relatively stable. Other branches are feature branches. This is documented in RELEASE_PROCESS.md and CONTRIBUTING.md. Since we tag each version, we don't really need the master branch, I'm considering to remove it. For your feature this means that you shall create a pull request to merge your master branch into the develop branch on s://github.com/kaaholst/android-squeezer. An initial comment is that the view menu is only accessible from JiveItemListActivity, so not from NowPlayingActivity, where the new settings are active. What I meant with my previous comment, was to implement something similar to plugin_list_menu.xml, but so it can be set from now playing.

schup011 commented 1 month ago

OK, got it. Give me some time....

schup011 commented 1 month ago

No worries, git can be a mouthful! master is the name the default git branch name. In Squeezer we use it to reflect what is currently in production. The develop branch contains changes that will be in the next version, and it shall be relatively stable. Other branches are feature branches.

I just did not understand why my local git did not commit my changes to my SongInfo branch and created a Master branch instead, but anyway, my changes are inside my Master branch.

Honestly, for me it is a little bit confusing that in Squeezer there are a couple of places for settings, like the "Preferences", then the "View" settings on the ListView, and now some new "Track Info" settings in NowPlaying. But I am sure you considered and discussed that already long time ago and I will implement it as you want it to have.

kaaholst commented 1 month ago

I'm not completely happy with it, either, I just haven't found a better solution. The basic idea is that settings should be available from where you would use them. E.g. player settings are on the players page, alarm clock settings on the alarms page, and for volume settings you have to open now playing, bring up the volume control panel, and then select the settings. There is also the confusion between settings from the home page which are for the server (LMS), and (most of) the above which are for the Squeezer app.

In general, I think it's best to avoid settings altogether, if there is a setting which would fit all users. This seems to be the approach by most Google apps. I'm not particularly fond of that either, at least not for all use cases.

If you have any ideas, I'm interested to hear it. For this feature, I think it's best just to create settings for the now playing layout in line with what we have, and then maybe later look into how to organize settings in a separate issue.

schup011 commented 1 month ago

Totally agree - this is rather a general consideration and nothing particularly connected with this new feature.

I have done the changes and I am ready to create the pull request. Do you want to check before or should I just proceed?

kaaholst commented 1 month ago

Please proceed, then we can use the GitHub support for commenting the pull request

schup011 commented 1 month ago

Pull request is here: Pull request

schup011 commented 2 weeks ago

Pull request has been merged.