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

Refactor code for displaying track lists. #182

Merged
merged 2 commits into from
Mar 16, 2016

Conversation

jcass77
Copy link
Member

@jcass77 jcass77 commented Mar 10, 2016

This PR represents the start of an effort to consolidate how lists of tracks are retrieved and displayed in the UI:

  • fixes an issue that caused the popup menu in the browse pane to render differently than the others, or not at all. Fixes Can't use contextmenu in browse pane #126.
  • further refactoring of renderSongLi. There's more work to be done here as the various play... functions can also probably be refactored and consolidated. I'd eventually like to get to a point where we only have one list of tracks that can be cached and used in all of the panes. But that is a PR for another day.
  • fixed a small issue with the default images for artists and albums sometimes not displaying if they cannot be retrieved from last.fm.
  • browsing using the local or file extensions will now also render album and artist details, which fixes Track artist is not displayed in browse results #99. It works well but the performance impact is noticeable for lists containing more than a hundred tracks or so. We might eventually either make this a configuration setting, or include a button on-screen that the user can use to toggle the details for any TrackRefs. Ultimately, this could also be used to sort by artist or album on the fly.

The rules for displaying album details are currently as follows:

  1. if we only have one track in the list, then show its album details
  2. if we have more than one consecutive track from the same album, then add an album header above the tracks. Album details are not repeated for each of the album's tracks.
  3. if we do not have any album info available or consecutive tracks are from different albums, then display a think grey divider between the tracks.

This PR also paves the way for fixing #69 and #96.

@jcass77
Copy link
Member Author

jcass77 commented Mar 10, 2016

Just a note that I have pushed some more changes which ensures that the loading popup is displayed until after all of the album details have been retrieved.

Also fixes an issue where the album details for the first track in the list was not being rendered.

@jcass77
Copy link
Member Author

jcass77 commented Mar 12, 2016

I've pushed a00754d which ensures that each track name is proceeded with an icon (except if part of a multi-track album) and a space character.

@jcass77
Copy link
Member Author

jcass77 commented Mar 12, 2016

jcass77@60041c1 refactors and consolidates how the 'Back' button is rendered at the top of track lists, and how messages are displayed when a folder does not contain any tracks.

jcass77@1c1fbee makes it more obvious if searches came up empty by showing 'No tracks found...' in the results table.

I'll run a few more tests and merge this tomorrow (Sunday) morning if there are no further inputs from anyone.

@kingosticks
Copy link
Member

I'm away this weekend so I cannot try this.

@jcass77
Copy link
Member Author

jcass77 commented Mar 14, 2016

5437205 optimises the jQuery selectors to use identifiers instead of attributes. Lists with roughly 2 000 tracks are now rendered about 80 times faster.

Also fixed a small issue with play icons not being updated in the #albumstable, and replaced string references with their JavaScript constants.

@jcass77
Copy link
Member Author

jcass77 commented Mar 16, 2016

I've been using this for a few days without any apparent issues. I'll merge later tonight if there are no objections.

jcass77 added a commit that referenced this pull request Mar 16, 2016
Refactor code for displaying track lists.
@jcass77 jcass77 merged commit df0775e into pimusicbox:develop Mar 16, 2016
@jcass77 jcass77 deleted the fix/99_browse_track_artist branch February 5, 2017 01:32
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.

Can't use contextmenu in browse pane Track artist is not displayed in browse results
2 participants