[scott967]

I found a bug, actually it pre-dates this PR.  When I go to music library nodes in music window and press <i> info key on a selected listitem:

For genres Container.Content(genres) I get musicinfo dialog for an album
For years Container.Content(years) I get a busy dialog and have to escape out of it
For sources Container.Content(sources) I get musicinfo dialog for an album
For roles Container.Content(roles) I get nothing (which appears to be correct to me?)

Did some quick testing, and it seems OK back on the alpha 1 release.

scott s.
.

[DaveBlake]

Thanks for this @scott967, I have split into its own thread so I can see it more clearly.

I am already aware that <i> gets different behaviour to the context menu "Information" item, but could not see way to make them consistent yet. Historically the implementation divereged, it is on my list to fix.  Really <i> should be a keyboard short cut to the context menu action, hence only function when that item also has that context menu item available and do the same action i.e. bring up the same info dialog.

Did some quick testing, and it seems OK back on the alpha 1 release.

My info dialog refactor may have modified something that was indirectly limiting when the dialog gets displayed. I will see if I can fix that even if I can't bring <i> and context menu into using the same code in the way they should be doing.

[scott967]

Interesting, when you look in sources, roles, or genres nodes and open the context menu on an item, it only offers Queue/Play/Add to Favourites but for an item in the years node, it also offers Information.   Just like using the info key, selecting information from the context menu results in unending busy dialog.

scott s.
.

[DaveBlake]

@scott967 this was an interesting little issue, fixed by PR14364, too late for Beta1 release but I will set a test build going.

The key issue is that that not all items with MusicInfoTags are songs, albums or artists - the other nodes use MusicInfoTag too (and some set ID). The years node items were being mistaken for songs, and genres were tripping artist art lookup! Glad you spotted the oddities, a good catch.

EDIT: test build for Win64 20180828-95c860f3-PR14364

[scott967]

Every thing seems to work as expected (both context menu info and action info via keypress).  I still get a crash when sending info action on a music video that's listed in musicplaylist window media container, but I guess that's expected.  OK to merge IMHO.

scott s.
.

[DaveBlake]

Thanks @Karellen and @scott967 for testing.

I still get a crash when sending info action on a music video that's listed in musicplaylist window media container, but I guess that's expected. 

I'm unable to reproduce that on a mixed playlist. The behaviour I see with the PR changes is that <i> does nothing, and "information" on the context menu displays the video info dialog but differently from what gets shown from the music video node. So inconsistent behaviour but no hang.

This makes sense as the info action message is processed by the music nav window, and that now does nothing unless the item media type is a song, album or artist. Then again I have only one test music video without anything related in the music db, and so maybe I'm doing it wrong. Can you clarify how I can reproduce the hang?

I do notice that the music video item has both "Play" on the context menu (from the context menu handler for video items) and "Play using..." (from the music nav window, as do the other songs on the current playlist). As you know music videos are poorly handled, and I have never had enough time to take the issues on. Having play or play using as context menu item on the current playlist seems kind of odd to me, so I am very tempted to leave it as it is for v18 at least. But I would fix the hang if I can catch it.

[DaveBlake]

@scott967 which skin were you using when you got the info action on a music video listed in musicplaylist window media container to hang?

I still have not managed to reproduce this behaviour (with the PR), but investigating why the Video Info Dialog shows genre, year, album title etc. when called from the music video node, and not when from the musicplaylist window, I think the info labels could be referencing a MusicInfoTag with empty fields rather than the VideoInfoTag that hold that data. That could possibly hang in some situations.

Yet to discover where this MusicInfoTag is created but not populated, and if music video items should or shoulldn't have one. I guess everything in the musicplaylist is expected to have a MusicInfoTag, and the GUI is checking that structure first to get info label values.

EDIT: OK, think I have this info dialog for music video on music current playlist issue sorted. I will raise a separate PR since I would like some more users with music videos to test and confirm I have not added regressions. PR14373

[DaveBlake]

Test build with info menu and action for music videos fixed is 20180829-abbe08dd-PR14373, this includes other fixes that have merged into master.

@scott967 see what you can do with this please.

[scott967]

Test build with info menu and action for music videos fixed is 20180829-abbe08dd-PR14373, this includes other fixes that have merged into master.

@scott967 see what you can do with this please.

 OK here's what I find (testing with estuary all settings at default except player/video/play next auto set to music video

Good news is now when music video items are in the musicplaylist window pressing <i> info key opens movieinformation dialog and doesn't crash.

The following problems were encountered:

1.  Music Window
navigating library nodes music video => genres (years, etc) <i> info key and context menu "information" opens movieinformation dialog (no info available)

2.  Music, Music Playlist and Music Playlist Editor windows
opening a mixed or musicvideo m3u playlist when focused on a musicvideo item, <i> info key does nothing and the context menu doesn't provide "information" as an option.

I am a getting a crash on exit but assume it's unrelated (no crash log or stack trace created).

scott s.
.

[DaveBlake]

Thanks for the feedback @scott967 , good that things are moving the right way.

1) "No information available" - I think that is about Estuary and just not having the kind of data it wants to show.
My initial test music video had the movieinformation dialog come up with "No information available" on the main container, showing genre and year at the top of the screen with the title. I added a lot of fake data to the musicvideo table, and the text in columns c05, c06 and  c08 were shown instead. I have no idea how those get populated in real use, and I don't care, I really don't want anything to do with tables with cXX column names.

Anyway, can you confirm that my explanation for what you experienced is correct?

2) Well at least it is consistent.
Very tempted to say it is by deisgn, I will take a look (music videos, mixed m3u playlists.... what are you getting me into?)

EDIT: Well the lack of <i> action, information menu and crash might be related to the m3u file including files that don't exist. If they do then I see the <i> action display the appropriate dialog on both music and music video items, but no information context menu for the music videos. So inconsistency there. Can you confirm that behaviour?

Oh and @black_eagle as a music video person perhaps you would like to have a look at this too

EDIT2: seems the inconsistent behaviour arises as CMusicInfoLoader::LoadItemLookup can not handle music videos. Not sure I can fix as part of beta, feels a little can-of-worms like.

EDIT3: FWIW I also get crashes on exit after I have been viewing m3u playlists. CGUIWindowMusicPlaylistEditor seems to be tring to destroy items that have already gone.

It's official I really don't like music videos and m3u playlists

[black_eagle]

Ah, guessing this is short videos as opposed to full on concerts ?  I'll do some testing and report back.  Unlikely to be over the weekend though...

[scott967]

Ah, guessing this is short videos as opposed to full on concerts ?  I'll do some testing and report back.  Unlikely to be over the weekend though...

 Yes, this is all related to treating music video items as song analogs.  Music videos can be displayed both in the videos window (and video playlist) and in the music window (and music playlist).  Most of the weirdness I have experienced is in the music and music playlist windows.  Back in Krypton and earlier, Kodi would show the song information for  music videos in the music* windows.  In Leia, that was fixed so music video would show the movie information instead, but that was causing a crash in the specific case of using the <i> info key in the music playlist.  (Note that Estuary / Confluence never dealt with the possibility of song information showing info for a music video.  Another aside is during playback of a mixed playlist, the video playlist only shows something if a music video is currently playing, in which case that video item is in the video playlist).

For m3u, I don't use them as when Kodi creates them, it will only write in 8-bit text encoding and as a result 90% of my library will provide invalid m3u entries (Kodi can read m3u encoded in utf-8 without problem, as long as there isn't a BOM in there as that causes Kodi to skip the line starting with BOM.  Windows, for reasons known only to MS, insists on writing a BOM in utf-8 encoded text files.)  I was curious though about how Kodi handles m3u playlists, partly because in testing I found that the player/video/play next auto setting when set to include music videos works in some cases, but in others music videos use the player/music/play next auto setting.  As far as not showing the movie information dialog for music videos, I don't care and doubt there is much (any?) user interest in it.

In Videos window when in the music video genres node (actually, all subnodes except titles) the <i> info key does nothing.  In Music window in Beta 1 <i> key works in titles, artists, and albums (and in that sense is value-added compared to videos window) though "information" isn't available in the context menu for MV artists or albums.

In the PR, now the <i> key opens movie information in all the music video nodes.  But I think the only info actually available is listitem.label.  Core provides plot, thumb, and cast but these are empty in this context.  I assume this new behavior is a side effect of fixing the <i> key crash.

scott s.
.

[DaveBlake]

As far as not showing the movie information dialog for music videos, I don't care and doubt there is much (any?) user interest in it.

Yeap, that seems about it.

My take from the feedback here is that what I have done is an improvement as it fixes the crashes, so I will merge the second PR. More could be done to make the visibility of the information context menu consistent, but there is also a lot more related work to be done to move the rest of the context menu items into the context menu handler approach. This was introduced back in 2016 but only partially implemented (hence the inconsistencies), and someone can take it on and implement fully for v19. I still have not figured out quite how it was intended to work given only existing partialy implemented code to go on.