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

View Actions for History panel #17

Merged
merged 13 commits into from
Feb 18, 2021
Merged

View Actions for History panel #17

merged 13 commits into from
Feb 18, 2021

Conversation

jkmartindale
Copy link
Collaborator

@jkmartindale jkmartindale commented Feb 17, 2021

Resolves #8 and partially addresses #14

Actions:

  • Add track to queue
  • Play track immediately
  • Refresh panel (gets new history entries)
  • Load more... button (gets older history entries)

Miscellaneous:

  • Filter actions by track context (instead of hardcoding a panel ID)

This name is shorter, easier to reason around (couldn't decide between)
calling things "recent" or "recents"), and reflects what Spotify shows
in their official clients.
Tracks in a TreeView have no children, so there is nothing to implement.
The commands contribution point is only needed for user-facing commands
accessible via the Command Palette. The internal actions for track play/
pause are useless to the end user, so remove them.
The Spotify Web API does not allow getting track history with both after
and before cursors set. after is used for getting newer history entries
and before is used for older history entries. This requires two ways of
updating the HistoryProvider track cache, so the logic was moved out of
the getChildren() function and into loadNewTracks(). getChildren() now
just returns the track cache.
This is done with the magic of setting Track.contextValue to "track" and
then showing actions when "viewItem == track"
Shout-out to Kshitijaa for showing me that
1) This is possible
2) This is cool and epic encapsulation
@jkmartindale jkmartindale marked this pull request as ready for review February 17, 2021 23:42
Copy link
Owner

@larkinds larkinds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Owner

@larkinds larkinds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: should we remove the //server.close(); on lines 68 and 76?

@larkinds larkinds merged commit 1bda13e into staging Feb 18, 2021
@jkmartindale jkmartindale deleted the feature/history branch February 19, 2021 00:19
@jkmartindale jkmartindale mentioned this pull request Feb 19, 2021
4 tasks
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.

2 participants