Skip to content

add hotkey to copy url of currently playing track#156

Merged
Rigellute merged 6 commits into
Rigellute:LennyPenny-clipboardfrom
LennyPenny:master
Nov 21, 2019
Merged

add hotkey to copy url of currently playing track#156
Rigellute merged 6 commits into
Rigellute:LennyPenny-clipboardfrom
LennyPenny:master

Conversation

@LennyPenny
Copy link
Copy Markdown
Contributor

Hey,
first of all: Awesome project!

I implemented a new feature that copies the url of the currently playing track when you press a button (c by default).

This is my first time using rust, so if you find any mistakes or weirdness, let me know and I will fix the issues.

I hope I found all the relevant sections I needed to adjust.

@LennyPenny
Copy link
Copy Markdown
Contributor Author

LennyPenny commented Nov 19, 2019

it seems like some library dependency forclipboard is missing from the github action runner - any tips on how to add that to the github action runner?

Copy link
Copy Markdown
Owner

@Rigellute Rigellute left a comment

Choose a reason for hiding this comment

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

Great work! This is a nice feature that I've often wanted.

Left a few comments about implementation, but looks good 👍 .

Perhaps in the future we could throw up a selection menu that lets the user pick which link they'd like to copy - track/album/playlist/artist

Not sure what's wrong with CI, will investigate.

Comment thread src/main.rs

app.spotify = Some(spotify);

app.clipboard_context =
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could/should the clipboard_context be initialised in app.rs? Or is it easier to handle the error from main?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Problem is that currently app initialization cannot fail. If we make it failable we will have to adjust every test. That would touch almost every file, so I refrained from doing so

Comment thread src/ui/help.rs
Comment thread src/app.rs
}

pub fn copy_song_url(&mut self) {
let clipboard = match &mut self.clipboard_context {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks like this could be using an if let?

if let Some(clipboard_context) = &mut self.clipboard_context {
  ...
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used an "early return" here to save an indentation level, do you want me to change it?:)

@LennyPenny
Copy link
Copy Markdown
Contributor Author

LennyPenny commented Nov 20, 2019

regarding the ci runner: I think the xorg-dev package is missing from the runner (I have no idea how one would add that)

@Rigellute Rigellute changed the base branch from master to LennyPenny-clipboard November 21, 2019 09:21
@Rigellute
Copy link
Copy Markdown
Owner

Going to merge this into a side branch so I can test CI/CD fix you suggested

@Rigellute Rigellute merged commit 490618e into Rigellute:LennyPenny-clipboard Nov 21, 2019
@Rigellute Rigellute mentioned this pull request Nov 21, 2019
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