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.

Sign up for GitHub

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

Jump to bottom

fix GUI states of Library feature buttons #2628

Merged
merged 14 commits into from Apr 15, 2020
Merged

fix GUI states of Library feature buttons #2628

merged 14 commits into from Apr 15, 2020

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 4, 2020 •
edited

fixes https://bugs.launchpad.net/mixxx/+bug/1854160

Analyze
Analyze button is now disabled by default until tracks are selected.
It can now be checked which also applies the 'active' style from qss when analysis is running.
(before, the enabled state was not synchronised until you changed the view from New to All)
The table model is now loaded when you switch to Analysis for the first time, not on skin load.

Recording
The Recording toggle and the info labels are now always synced to actual recording state and styled correctly, nomatter if the recording was toggled via Ctrl+R, menubar toggle, toolbar toggle or Rec toggle itself, even after changing the skin.
I've split up the GUI labels to emphasize the file name.
image

In DlgAutoDJ I switched to clicked() signal to prevent any issues like with Recording, and applied the new signal/slot syntax.

@ronso0
make Anylysis button checkable, unchecked by default
8206d07
@ronso0
DlgAutoDJ: use clicked() signal for all QPushButtons
7b8d74b 
This prevents inifit loop issues when real click events and
programmatic QPushButton::setChecked() calls conflict:
* clicked(bool checked) is only emitted on mouse clicks
* toggled(bool checked) is emitted both on mouse clicks and setChecked() calls
@ronso0
DlgRecording: use QPushButton::clicked() signal instead of toggled()
6035a24
@ronso0
DlgRecording: separate Rec toggle & button update
db17fd1 
Redirect toggle request to RecordingManager::slotToggleRecording()
and let slotRecordingEnabled() handle the button update.
@ronso0
DlgRecording: use unambiguous slot names
2e66652
@ronso0
recordingmanager.cpp: apply new signal/sot syntax
6be0ef8
@ronso0 ronso0 added this to the 2.3.0 milestone Apr 4, 2020
@ronso0 ronso0 added this to In progress in 2.3 release via automation Apr 4, 2020
@ronso0 ronso0 moved this from In progress to In Review in 2.3 release Apr 4, 2020
@ronso0 ronso0 requested a review from Holzhaus April 7, 2020 12:04
Holzhaus
Holzhaus requested changes Apr 9, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM so far, thank you. Can you also disable the AutoDJ button when the AutoDJ queue is empty?

Also, where's the analyze button? I didn't test it because I couldn't find it 🤷‍♂️

src/library/recording/dlgrecording.cpp Outdated Show resolved Hide resolved
src/library/recording/dlgrecording.cpp Outdated Show resolved Hide resolved
src/library/recording/dlgrecording.cpp Outdated Show resolved Hide resolved
ronso0 and others added 3 commits April 9, 2020 16:52
@ronso0 @Holzhaus
dlgrecording.cpp: move the parentheses out of the translation string
4abb53d 
Co-Authored-By: Jan Holthuis <holthuis.jan@googlemail.com>
@ronso0 @Holzhaus
dlgrecording.cpp: remove double parantheses
0c8bfff 
Co-Authored-By: Jan Holthuis <holthuis.jan@googlemail.com>
@ronso0 @Holzhaus
dlgrecording.cpp: remove double parantheses
022d060 
Co-Authored-By: Jan Holthuis <holthuis.jan@googlemail.com>
@ronso0
Copy link
Member Author

ronso0 commented Apr 9, 2020

Also, where's the analyze button? I didn't test it because I couldn't find it

In the Analyze feature:
image

Can you also disable the AutoDJ button when the AutoDJ queue is empty?

Hmkay.. if the queue is empty && if "Enable random track addition" is disabled

@ronso0
Copy link
Member Author

ronso0 commented Apr 9, 2020

Can you also disable the AutoDJ button when the AutoDJ queue is empty?

Hmkay.. if the queue is empty && if "Enable random track addition" is disabled

Looking closer, we'd need to connect DlgPrefAutoDJ::slotApply() to DlgAutoDJ::autoDJStateChanged because the button has to be updated when "Enable random track addition" is toggled. IMO this requires too many includes (and too much effort), regarding that it's obvious the button wouldn't do anythig anyway when the queue is empty.
Do you think that's worth it?

@Holzhaus
Copy link
Member

Holzhaus commented Apr 9, 2020

Shouldn't there be a connection anyway? How is AutoDJ notified that the value has been changed while it's running?

@ronso0
Copy link
Member Author

ronso0 commented Apr 9, 2020

AutoDJProcessor reads the config when a button is pressed, and when the next track is requested.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 9, 2020

Anyway, would have been nice but maybe this is more trouble than it's worth.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 9, 2020

AutoDJProcessor reads the config when a button is pressed, and when the next track is requested.

Ok. Then just leave it as-is.

@ronso0 ronso0 mentioned this pull request Apr 10, 2020
Closed
@Holzhaus
Copy link
Member

LGTM. Test fails are unrelated.

@Holzhaus Holzhaus merged commit 1acfda2 into mixxxdj:master Apr 15, 2020
2.3 release automation moved this from In Review to Done Apr 15, 2020
@ronso0 ronso0 deleted the lib-button-states branch April 15, 2020 09:11
@mixxxbot mixxxbot mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Holzhaus requested changes

Assignees
No one assigned
Labels
library ui
Projects
2.3 release
  
Done
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ronso0 @Holzhaus @Be-ing