Buggy playlist focus changing behaviour

shadeMe commented 4 years ago

My current setup looks like this: Bottom/middle playlist - Show active playlist, don't follow now playing Right playlist - Show now playing playlist, follow now playing

Incorrect behaviour: When both bottom and right playlists are currently showing the now playing playlist, changing the selected item automatically updates the state of and brings the item into focus in both bottom and right playlists.

Expected behaviour: Changing the selection in either playlist shouldn't affect the other.

Additional Info: I actually fixed this in my local build several months ago but forgot to commit the changes. Unfortunately, I couldn't reconcile my local build with the changes from upstream since and ended up losing track of what I had changed. If I recall correctly, it had to do with the call to set the active item for a given playlist using the playlist manager global. This had the side-effect of setting the the active item on all playlist views if they were displaying the same playlist.

I tried to bugger about a bit with the latest code, but it seems like I need to spend a fair amount of time to get my bearings. So, I hope you'll have better idea of where to look and what to fix.

PS: I'm using the latest tree that was merged into my local branch yesterday.

Ottodix commented 4 years ago

Hi, A click in a playlist changes the focused item. A change of focus trigger a callback, on_item_focus_change(), and then the playlist scrolls to the newly focused item. In this function, there is some code in order to scroll to the focused track, inside a if(!g_rbtn_clic) {}. You can deactivate this code.

Maybe I'll remove this "scroll to focused track" code, but I need to test more, maybe it breaks some other things.

Ottodix commented 4 years ago

I removed this "scroll to focused track" code in my latest commit. I'll fix the eventual issues later, didn't find any so far

shadeMe commented 4 years ago

Thanks. The new behaviour seems to be the following (when the bottom and right playlist are showing the now playing playlist and a track is playing):

Start state: Both bottom and right views show the currently playing track (scrolled into view and selected/has focus)
- Select another track in bottom view:
  - Selection in the right view changes, but it doesn't get scrolled into view. Ideally, we want both views to have their own state and stay independent of each other, but this is an okay compromise.
- Select another track in the right view:
  - Same as above when the group header in the bottom view are not set to auto-collapse. Otherwise, if the new selection is in another album, the currently expanded group header is collapsed. This needs to be fixed.
- Currently playing track gets changed:
  - Same behaviour as above if headers are set to auto-collapse.

Ottodix commented 4 years ago

Ok, for the auto-collapse change, yep, not perfect, I'll change that. For the selection change, it will remain. When you select a track, the selected state is sent to foobar API, and so it changes on both playlists. It would be possible to code a selection system independant from foobar API, but I don't think it's a good idea to do that, it won't interface well with others components.

shadeMe commented 4 years ago

Fair enough. Another side-effect/regression I just noticed is that when expanding (auto-)collapsed headers in the bottom view, the expanded album/group is no longer scrolled/centered into view like it used to be. This particularly jarring when you expand a long album, scroll down until is well out of the view and expand a smaller album/group close to the top of the viewport. This collapses the previous album, which has the side-effect of changing the scroll offset and causing the newly selected album to consequently scroll (up and) out of view.

Ottodix commented 4 years ago

Probably fixed with my latest commit. (I didn't test it a lot, I let you do that)

shadeMe commented 4 years ago

Not entirely, I'm afraid. The latest changes seems to have made the focus-change behaviour more erratic (with auto-collapsed headers, at least). Issues I've noticed so far:

Selecting another track in the right view does reset the bottom view's selection, but the viewport doesn't change. This is correct behaviour. However, this allows the bottom view to have multiple expanded headers as opposed to just one. Steps to reproduce: Expand a header in the bottom view, select some track in the right view, expand another header in the bottom view, and both those headers stay expanded.
The focus/selection/viewport still gets reset when a track change occurs (user-induced or automatic).
Expanding an album/header in the bottom view with another expanded header out-of-view still causes "jumping" but this time in the opposite direction. Don't remember if this was the original behaviour, but I'd say we want the expanded header to not move out from under the cursor during expansion.

Ottodix commented 4 years ago

About the jump, as long as it stay in the viewport, it's ok for me. About the other part, I did a change. But actually it breaks other things, like the keyboard navigation.. I'm not interested enough to spend time on this, I don't use this autocollapse feature myself.

shadeMe commented 4 years ago

Fair enough. I'll see if I can fix it from my side and send you a PR.

shadeMe commented 4 years ago

Can you reopen this issue for the time-being? On a different note, do you have any tips for debugging the scripts? Right now, I just print traces to the console and stumble around. Is there anyway to attach a JS debugger to the foobar/spidermonkey process? If not, is there some API function that prints out the VM's callstack?

Ottodix commented 4 years ago

I do the same, print to the console. You'll have to ask theqwertiest: https://github.com/theqwertiest/foo_spider_monkey_panel

shadeMe commented 4 years ago

Removing the SetPlaylistFocusItem call in WSHsmoothplaylist_trackinfo:oBrowser.showNowPlaying fixes the issue with the selection/focus change in both playlist panels. https://github.com/Ottodix/Eole-foobar-theme/blob/495b226e7e6c6b67ca42841691dd3ef7573bbf85/themes/eole/js/WSHsmoothplaylist_trackinfos.js#L1688

Incidentally, I believe this was indeed my original fix. My suggestion would be to rollback your most recent changes that broke keyboard navigation and to make the above change instead. I have yet to discover any unwanted side-effects so far.

Ottodix commented 4 years ago

it doesn't fix the issues which you listed on this ticket.

I explained the issue in my first comment: plman.SetPlaylistFocusItem() is used everywhere across this playlist script. For keyboard navigation, when you expand a group, etc. And then, when you change the focus, it trigger the callback function on_item_focus_change(), and then the playlist scrolls to the newly focused item. As this callback is triggered on all playlists panel when the focus change, it scroll to focused item on all playlists panel if they are displaying the same playlist.

So i added a flag, which say "scroll to focused item only if it's from a focus change triggered from this playlist instance"

shadeMe commented 4 years ago

it doesn't fix the issues which you listed on this ticket.

Not all of them but just this one: The focus/selection/viewport still gets reset when a track change occurs (user-induced or automatic).

I pulled your latest changes, and I'm afraid it has broken the header auto-collapse function even more. Now expanded groups get collapsed (seemingly) randomly after scrolling and when a track changes. Also, it seems like the playlist views are getting redrawn more often now, causing micro-stuttering when changing selection/expanding headers.

As for your comment on SetPlaylistFocusItem, I understand that it's used extensively in the codebase and that the call on line 1688 is intended. However, I still find that commenting out that call is the most consistent fix for the incorrect behaviour. Granted it can potentially break keyboard-navigation, but since that is not important for me, I'll just stick to it for my local build.

Regardless, this issue can stay closed now. Thank you taking the time to look into this - Much appreciated.

Ottodix commented 4 years ago

Yep, I know, it's not perfect. Probably I'll take a look when I'll be in a better mood.

Ottodix commented 4 years ago

Now, afaik, it work perfectly (hmm... I'm pretty sure that you will find issues...but I didn't myself!)

shadeMe commented 4 years ago

Now, afaik, it work perfectly (hmm... I'm pretty sure that you will find issues...but I didn't myself!)

Haha, I'm afraid you were right. On my end, switching tracks causes the currently expanded group in the bottom playlist (if it's not the header of the currently playing track's album) to collapse after a couple of seconds. The following lines are printed out to the console when this happens:

--> populate Smoothplaylist with PlayingPlaylist call_id:14 Parent panel:MainPanel
--> populate Smoothplaylist with ActivePlaylist call_id:14 Parent panel:MainPanel

Commenting out the SetPlaylistFocusItem line prevents this from happening, as far as I can tell. The above lines do get printed out, although, indicating that the view(s?) get repopulated, but the header says open most of the time (there are still instances when it gets collapsed, but I haven't been able to consistently reproduce it).

Ottodix commented 4 years ago

"switching tracks", isn't it related to playing a track? Doesn't it happens after 1 min of playing? The call_id there shows that a meta tag changed, and it could be related to the play count of the currently played track. It's updated after 1min of play

shadeMe commented 4 years ago

By "switching tracks", I was referring to the following: when the currently playing ends and the next track automatically begins, or when the 'Previous' or 'Next' controls are used to skip the currently playing track. The incorrect collapsing behaviour definitely doesn't take a minute - It's usually with the first 10-15 seconds after the switch. Scrolling the playlist during those 10-15 seconds seems to trigger the repopulation more quickly.

Ottodix commented 4 years ago

Did you install new components, something for playback stats, something which write a tag on the played file? A call to on_metadb_changed happened on your setup, but not on mine. Your issue may be gone with my latest commit, but I don't know why you've got this call to on_metadb_changed in the first place

shadeMe commented 4 years ago

Initial impressions do seem to indicate that your latest commit has mitigated the issue, but I'll keep an eye out all the same.

No, I haven't installed any new components recently. I have always had foo_playcount installed, though. My components:

Core (2019-11-28 12:47:26 UTC)
    foobar2000 core 1.5
foo_acfu.dll (2019-11-29 20:49:29 UTC)
    Auto Check for Updates 0.3
foo_albumlist.dll (2019-11-28 12:46:52 UTC)
    Album List 4.7
foo_audioscrobbler.dll (2012-06-23 16:23:05 UTC)
    Audioscrobbler 1.4.7
foo_cad.dll (2020-05-27 09:02:11 UTC)
    CD Art Display 1.0.2
foo_cdda.dll (2019-11-28 12:46:48 UTC)
    CD Audio Decoder 3.0
foo_chacon.dll (2014-02-09 06:41:30 UTC)
    Chacon 3
foo_comserver2.dll (2006-07-31 19:13:20 UTC)
    COM Automation server 0.7 alpha 6
foo_converter.dll (2019-11-28 12:46:44 UTC)
    Converter 1.5.4
foo_convolve.dll (2019-12-23 13:40:47 UTC)
    Impulse Response Convolver 0.3.3
foo_customdb.dll (2013-07-20 15:38:40 UTC)
    Custom Database 0.0.9a
foo_discogs.dll (2020-05-06 17:05:09 UTC)
    Discogs Tagger 2.23
foo_dsp_eq.dll (2019-11-28 12:46:46 UTC)
    Equalizer 1.2.1
foo_dsp_std.dll (2019-11-28 12:46:34 UTC)
    Standard DSP Array 1.3.2
foo_dynamic_range.dll (2013-01-22 18:49:33 UTC)
    Dynamic Range Meter 1.1.1
foo_enhanced_playcount.dll (2019-11-29 20:19:36 UTC)
    Enhanced Playback Statistics 4.2.1
foo_exvar.dll (2008-06-13 17:12:56 UTC)
    Extended Variables 0.3.1
foo_facets.dll (2012-05-25 12:26:43 UTC)
    Facets 1.0
foo_fileops.dll (2019-11-28 12:46:42 UTC)
    File Operations 2.4
foo_foobarCon.dll (2014-05-05 12:31:50 UTC)
    HTTP Control for FoobarCon 0.97.28-fc
foo_freedb2.dll (2019-11-28 12:46:52 UTC)
    Online Tagger 0.7
foo_input_monkey.dll (2019-11-29 20:20:15 UTC)
    Monkey's Audio Decoder 2.3.1
foo_input_std.dll (2019-11-28 12:47:14 UTC)
    FFmpeg Decoders 3.4.6-0ac9001
    Standard Input Array 1.5
foo_lastfm_playcount_sync.dll (2019-12-23 13:40:38 UTC)
    Last.fm Playcount Sync 1.0.1
foo_masstag.dll (2019-11-29 20:27:15 UTC)
    Masstagger 1.8.5
foo_musicbrainz.dll (2020-05-27 09:06:36 UTC)
    MusicBrainz Tagger 0.4.6
foo_nds.dll (2018-11-02 23:53:45 UTC)
    No Display Standby 1.1.2
foo_play_next.dll (2019-11-29 20:31:06 UTC)
    Play Next 0.2.1
foo_playcount.dll (2019-11-01 22:16:17 UTC)
    Playback Statistics 3.0.3
foo_playlist_attributes.dll (2019-11-29 20:29:19 UTC)
    Playlist Attributes 0.5.5
foo_pqview.dll (2012-12-01 19:08:19 UTC)
    Playback Queue Viewer 0.2
foo_queuecontents.dll (2012-12-20 12:37:18 UTC)
    Queue Contents Editor 0.5.1
foo_quicktag.dll (2012-06-07 14:40:49 UTC)
    Quick Tagger 1.0.3
foo_rgscan.dll (2019-11-28 12:46:46 UTC)
    ReplayGain Scanner 2.3
foo_sanitizer.dll (2019-11-29 20:28:10 UTC)
    Tag Sanitizer 1.2.1
foo_scheduler.dll (2020-05-06 17:05:09 UTC)
    Scheduler 4.19
foo_spider_monkey_panel.dll (2020-07-06 21:34:55 UTC)
    Spider Monkey Panel 1.2.4-beta+1f130249
foo_stop_after_queue.dll (2018-11-02 23:53:45 UTC)
    Stop After Queue 1.1.1
foo_stopafteralbum.dll (2013-03-23 15:44:52 UTC)
    Stop after album 3.5
foo_tagbox.dll (2011-03-11 06:53:00 UTC)
    TagBox 0.212
foo_ui_columns.dll (2020-07-06 21:34:56 UTC)
    Columns UI 1.3.0
foo_ui_hacks.dll (2019-11-01 22:16:18 UTC)
    UI Hacks 2013-02-14
foo_ui_std.dll (2019-11-28 12:47:06 UTC)
    Default User Interface 1.5
foo_uie_console.dll (2019-11-29 20:31:51 UTC)
    Console panel 0.5
foo_uie_elplaylist.dll (2019-11-01 22:16:18 UTC)
    ELPlaylist 0.6.9.1.2(beta)
foo_uie_eslyric.dll (2019-11-01 22:16:18 UTC)
    ESLyric 0.3.6 
foo_uie_panel_splitter.dll (2019-11-01 22:16:18 UTC)
    Panel Stack Splitter 0.3.8.3(alpha)
foo_unpack.dll (2019-11-28 12:46:44 UTC)
    ZIP/GZIP/RAR Reader 1.8
foo_utils.dll (2010-07-20 19:09:44 UTC)
    Playlist Tools 0.6.2 beta 6
foo_vis_shpeck.dll (2019-11-01 22:16:18 UTC)
    Shpeck - Winamp vis plugins wrapper 0.3.7
foo_vis_vumeter.dll (2019-11-01 22:16:18 UTC)
    VU Meter 2013-02-16
foo_vst.dll (2011-03-05 06:19:04 UTC)
    VST 2.4 adapter 0.9.0.3

Ottodix commented 4 years ago

OH my... don't expect me to take a look at all this components ^^

shadeMe commented 4 years ago

OH my... don't expect me to take a look at all this components ^^

Nah, I just posted that for reference. Rest assured, heh.

Have found another anomaly - Under whatever circumstances, the entire skin gets randomly reloaded, i.e., all Spidermonkey panels get reloaded. This causes the bottom playlist view to rest its state. The right playlist view, however, remains intact (or, at least, its state gets restored somehow). The following entries are found in the console when this happens:

Spider Monkey Panel v1.2.4-beta+1f130249 (TitleBar v1.2.3b16 by Ottodix): initialized in 219 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (CoverPanel v1.2.3b16 by Ottodix): initialized in 30 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (Controls v1.2.3b16 by Ottodix): initialized in 50 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (LibraryFilter v1.2.3b16 by Ottodix): initialized in 37 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (LibraryTree v1.2.3b16 by Ottodix): initialized in 107 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (GraphicBrowser v1.2.3b16 by Ottodix): initialized in 231 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (SmoothPlaylistManager v1.2.3b16 by Ottodix): initialized in 32 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (PlaylistFilter1 v1.2.3b16 by Ottodix): initialized in 28 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (PlaylistFilter2 v1.2.3b16 by Ottodix): initialized in 24 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (PlaylistFilter3 v1.2.3b16 by Ottodix): initialized in 26 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (PlaylistHeader v1.2.3b16 by Ottodix): initialized in 22 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (BottomPlaylist v1.2.3b16 by Ottodix): initialized in 33 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (ArtistBio v1.2.3b16 by Ottodix): initialized in 74 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (LyricsTitle v1.2.3b16 by Ottodix): initialized in 32 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (CoverPanel v1.2.3b16 by Ottodix): initialized in 27 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (NowPlayingPlaylist v1.2.3b16 by Ottodix): initialized in 20 ms
Spider Monkey Panel v1.2.4-beta+1f130249 (GraphicBrowser v1.2.3b16 by Ottodix): initialized in 87 ms
--> populate Filter order:0 parent panel:Library call_id:0
--> populate Library Tree in: 0.111 seconds call_id:on_init
--> populate GraphicBrowser sorted:false call_id:13
--> populate Filter order:0 parent panel:Playlists call_id:0
--> populate Filter order:1 parent panel:Playlists call_id:0
--> populate Filter order:2 parent panel:Playlists call_id:0
--> populate Smoothplaylist with ActivePlaylist call_id:2 Parent panel:MainPanel
--> populate Smoothplaylist with PlayingPlaylist call_id:2 Parent panel:MainPanel
showNowPlaying_trigger
--> populate GraphicScreensaver sorted:false call_id:13
--> populate smoothPlaylistManager call_id:2
--> refresh MainPlaylist Header populate_list:true call_id:14

This appears to happen every 30 odd minutes, but it doesn't seem like the timing is based on an expired setTimeout call (it's not period, as far as I can tell).

Ottodix commented 4 years ago

Weird, probably something specific to your config, again.

shadeMe commented 4 years ago

Does anything look amiss in the above messages?

Ottodix commented 4 years ago

Really I've got no idea why it does that on your config

Ottodix / Eole-foobar-theme

Buggy playlist focus changing behaviour #81