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

Created new confirmation dialog; cleared some redundancy in dialog usage #2486

Merged
merged 3 commits into from Jul 23, 2017

Conversation

Projects
None yet
2 participants
@Meriipu
Contributor

Meriipu commented Jul 18, 2017

New confirmation dialog in qltk/msg.py for cancel/accept-dialogs that takes title, description, and button label.
This also changes the plugin prompts (for activating plugins on X many items) in songsmenu to use this for a tiny cleanup in code reuse.

I have a second branch that adds a confirmation prompt using this new dialog for removing tracks from the library that I could include in this request if that is desirable, or maybe in a second request (though it depends on this).

I noticed tests/test_qltk_chooser.py which I think could be used to put together some tests too

@Meriipu Meriipu force-pushed the Meriipu:prompts_cleanup branch from 3ec3fd3 to 10a8e9c Jul 18, 2017

@Meriipu

This comment has been minimized.

Contributor

Meriipu commented Jul 18, 2017

no idea how I missed it the first time, pretty sure I searched. I can not find any other ConfirmMulti-usages now except ConfirmMultipleSongsAction and ConfirmMultiPlaylistInvoke now though and those are defined elsewhere.

@Meriipu

This comment has been minimized.

Contributor

Meriipu commented Jul 18, 2017

although how do you run a plugin on multiple playlists?

though correct me if I misunderstand but there seems to be tests already for exceeding max invocations on at least songs tests/test_plugins_songsmenu.py and playlists tests/test_plugins_playlist.py ? And since the interface did not really change I suppose that is all fine still then ?

I can not find any tests for the album one.

@lazka

This comment has been minimized.

Member

lazka commented Jul 23, 2017

Not sure about the playlists. There is a plan to support selecting multiple playlists, so that might have been a forward-looking implementation.

Looks good. Although in the long run we'll have to move away from blocking dialog methods again as GTK4 wont have a run method for dialogs (no nested main loops).

@lazka

This comment has been minimized.

Member

lazka commented Jul 23, 2017

ok to merge?

@Meriipu

This comment has been minimized.

Contributor

Meriipu commented Jul 23, 2017

should be good from what I can tell unless replacing other dialogs too (to use the same class, instantiated through similar functions or something) is desirable

  • ConfirmMultipleSongsAction is the only one with the same ConfirmMulti-naming style left I can find, and it seems to add three buttons so I decided to leave that as is.

  • A few others use the delete icon, so maybe if icon to use is passed as argument

  • maybe qltk/ratingsmenu.py:ConfirmRateMultipleDialog

but yes other than that.

@lazka lazka merged commit 7132552 into quodlibet:master Jul 23, 2017

7 checks passed

ci/circleci: job.debian8.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.debian8.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.ubuntu16.04.py3 Your tests passed on CircleCI!
Details
ci/circleci: job.win32.py2 Your tests passed on CircleCI!
Details
ci/circleci: job.win32.py3 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Meriipu Meriipu deleted the Meriipu:prompts_cleanup branch Jul 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment