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

pop up Spinny (with cover) context menu at click location #2359

Merged
merged 2 commits into from
Nov 24, 2019

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 16, 2019

fixes https://bugs.launchpad.net/mixxx/+bug/1852829
"spinny context menu (cover art menu actually) pops up at wrong place"

Additionally, the menu now opens only if the Spinny can actually show the cover (instantiated with <ShowCover>true</ShowCover>.
I'm not going to fix another inconsistency here which shows the cover menu on top of the Spinny when the cover is empty, but in the full-size cover if there IS a cover.

@Holzhaus
Copy link
Member

Strange, I don't even get a context menu. When right-clicking the spinnies the full size covert art dialog opens.

@ronso0
Copy link
Member Author

ronso0 commented Nov 16, 2019

yeah, it's actually redirecting to the WCoverArt menu.
And, now that you mention it, IMO the menu should only pop up if <ShowCover> is set TRUE for the sppinny instance. Or we just leave it so it maybe increases the discoverability for spinnies WITH cover? idk..

@ronso0
Copy link
Member Author

ronso0 commented Nov 16, 2019

I decided to show the cover menu only if spinnies are instantiated with <ShowCover>true</ShowCover>.
IMO it's pointless to have the cover menu when the supplied actions like 'Choose new cover' etc. don't show any effect in the widget you just clicked.

@ronso0
Copy link
Member Author

ronso0 commented Nov 17, 2019

Strange, I don't even get a context menu. When right-clicking the spinnies the full size covert art dialog opens.

@Holzhaus
Now I see what you refer to,.. The Spinny right-click menu depends on wether the track actually has a cover image, not on wether the Spinny can show the cover.

  • track has no cover. right-click > cover menu above spinny
  • track has cover. right-click > full-size cover window with right-click cover menu

This is confusing.
Spinny interaction is be more predictable if the context menu only pops up if you can actually see your changes in the spinny, and always in the same place (right-click in cover window).
To achieve that the only thing to do is to show the default image if the track has no cover, in half-sized window maybe, something like 300 x 300px.

Any thoughts?

@daschuer
Copy link
Member

daschuer commented Nov 17, 2019

I would remove the menu completely.

It is not obvious that a spinny that represents the jogg wheel is responsible for setting a cover. We have already two other places to do that.

I can remember that we had a slip mode action on the right click in some earlier versions. This is a more reasonable thing.

Alternative I can think of a right click or even a left click menu on the title and artist.
Lable. There can we integrate a cover dialog as well.

If you like to keep the cover menu, I would remove the full size cover display.

What is the use case for a full size cover by the way. I am asking because if the pending HiDpi scaling issue.

@ronso0
Copy link
Member Author

ronso0 commented Nov 17, 2019

What is the use case for a full size cover by the way.

Covers are interesting, and IMO the most reasonable way to assign a memorable hmm.. token to intangible tracks in a virtual list a. For vinyl records you have the record sleeve, in Mixxx we should keep the full-size cover.

Of course, if the Spinny had a ad-hoc slip mode function on right-click (if implemented reasonably on touchscreens) we'd need to remove the full-size cover on right-click.

The cover on right-click makes sense IMO. When it's visible in the Spinny it should also be avalible in full size.
I'd appreciate the track property menu for various reasons, but as of now, no-one even started to work on that, probably because it's coupled to the library.
For 2.3 let's just tweak the appearance and menu a bit so the existing implementation becomes consistent in the next major release.

@daschuer
Copy link
Member

Ok, makes sense.

What is the use case for a full size cover by the way

Of cause, showing a cover in Mixxx has a high value.

My question was about the use case if the full size cover. On a ratio 1 screen it is displayed pixel exact. That's fine.

If we resize it by the device pixel ratio, it becomes blurry, small covers are still small.

Maybe a use case is to inspect the cover quality. In this case it should be pixel exact on any screen. Do you agree?

@ronso0
Copy link
Member Author

ronso0 commented Nov 17, 2019

I'm fine with how the cover art is diplayed on my 1366x768 screen. Apart from that, I don't really have an opinion to share in the #2247 discussion because I can't judge the FHD aspects and side effects of it.

I'd like to keep this PR about the menu and cover art display conditions only.

@ronso0
Copy link
Member Author

ronso0 commented Nov 18, 2019

Done, ready to merge.

@Holzhaus
Copy link
Member

LGTM.

@daschuer
Copy link
Member

Thank you. LGTM

@daschuer daschuer merged commit 6ec24b4 into mixxxdj:master Nov 24, 2019
@ronso0 ronso0 deleted the cover-menu-placement branch November 24, 2019 20:25
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.

3 participants