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

Add sharing functionality #67

Merged
merged 12 commits into from
May 10, 2019
Merged

Add sharing functionality #67

merged 12 commits into from
May 10, 2019

Conversation

Herbstein
Copy link
Contributor

@Herbstein Herbstein commented May 7, 2019

I do a lot of sharing of Spotify songs, and to make this my main client I need the feature. This implementation uses the key-bind m and the command share to copy the currently playing song to the system clipboard.

Creating a clipboard context for each time a share happens isn't great, but because sharing is (generally) a rare occurrence.

The new library is used by Mozilla in Servo so shouldn't be an issue to include.

Is copying the currently playing song the desired functionality, or would it be better to share the currently selected song? Maybe sharing the currently playing song only when used with a shift modifier. You could then modify the command to be either share selected or share current.

@im-n1
Copy link

im-n1 commented May 8, 2019

This is great just one question. Why is "sharing" under "m" and not under "s"?

@Herbstein
Copy link
Contributor Author

Because s is already taken. I didn't want to move that as my initial implementation, but I agree that it's the best place for it.

@hrkfdn
Copy link
Owner

hrkfdn commented May 8, 2019

Thanks for the PR! s would indeed be nice for sharing, but it's taken by save, which seems equally fitting 😛.

Other options:

  • y: as in yank/copy from vim.
  • x: no good reason to be honest, except it feels appropriate.

Do you want to re-map it before I merge it? I'm fine with it either way.

@Herbstein
Copy link
Contributor Author

I'll move it to x. There are already a few things down on the bottom row.

@Herbstein
Copy link
Contributor Author

Do you have a preference on saving the selected song vs currently playing?

@hrkfdn
Copy link
Owner

hrkfdn commented May 8, 2019

To the clipboard? I'd say the currently selected, otherwise one needs to first play a song to share the link. What do you think?

@Herbstein
Copy link
Contributor Author

I can definitely see both usecases. We could amend the command with a parameter. current for the currently playing and selected for the currently selected. The keybind x could then default to one of those, with Shift+x for the other option.

@hrkfdn
Copy link
Owner

hrkfdn commented May 8, 2019

I like the idea!

@Herbstein
Copy link
Contributor Author

I am struggling with finding out which track/element is currently selected. You could make it general to any selected "shareable" element, but right now I think that requires doing backwards parsing from Cursive.

@hrkfdn
Copy link
Owner

hrkfdn commented May 9, 2019

Commands are handled in their respective UI source files. The UI structs should have access to the currently selected track, e.g. ui/listview.rs. Have a look at the on_command handler. Hope this helps.

@Herbstein
Copy link
Contributor Author

That was indeed very helpful! Do you want me to move the entire share command into the ListView impls? I can't quite figure if I prefer handling the "generic" method in a generic place, and the specifics in a specific place, or having an entire command implemented in one place. Any thoughts on that?

@hrkfdn
Copy link
Owner

hrkfdn commented May 10, 2019

If the single handler doesn't get too convoluted, I'd prefer that, as new code readers might miss the specific implementations distributed across the files. However, if it results in a too large method, it would seem reasonable to split it. Just my two cents, though.

@Herbstein
Copy link
Contributor Author

One last thing. I come from a functional programming background. If the command option chaining is too big I can easily break it up if you prefer.

src/ui/listview.rs Outdated Show resolved Hide resolved
@hrkfdn
Copy link
Owner

hrkfdn commented May 10, 2019

Looks good to me. I commented a minor detail, after that I think we can merge it.

@Herbstein
Copy link
Contributor Author

I updated the readme. I don't think I have anything else to add.

@hrkfdn
Copy link
Owner

hrkfdn commented May 10, 2019

Great, thanks a lot for your contribution. Merged!

@hrkfdn hrkfdn merged commit 5eff818 into hrkfdn:develop May 10, 2019
mtshrmn referenced this pull request in mtshrmn/ncspot Sep 8, 2020
Add sharing functionality
@inwwin
Copy link

inwwin commented Mar 7, 2021

@Herbstein I cannot use this feature in Wayland. Is this suppose to work in Wayland?

Edit: It seems the upstream project has not yet support Wayland, never mind.

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.

None yet

4 participants