WTrackMenu: Display a modal progress dialog #2899

uklotzde · 2020-06-23T15:29:58Z

!!! Target branch is 2.3 although assigned to the 2.4.0 milestone. Don't merge before joint decision !!!

~~- [ ] Finalize & approve software design~~ Good enough for now. We can change it when needed.

Not perfect and comes with some drawbacks, but still a huge usability improvement:

Display a modal progress dialog while batch processing multiple tracks
Allow to safely abort batch processing
Remove obsolete and performance killer method WTrackMenu::getTrackPointers()

Any ideas on how to implement a batch processing framework with less boilerplate and without the (implicit) usage of QCoreApplication::processEvents()?

Be-ing · 2020-07-03T23:40:20Z

Thank you for working on this tricky optimization. A popup dialog is an easy solution for the GUI design and I'm okay with merging that, but long term it would be better to design a place in the main window for a progress bar. That could also be reused for removing the Analyze library feature and the library scanner popup window.

src/library/trackmodeliterator.h

src/util/itemiterator.h

src/library/trackprocessing.cpp

Be-ing · 2020-07-04T00:04:19Z

src/library/trackprocessing.cpp

+    int estimatedTotalCount =
+            pTrackPointerIterator->estimateItemsRemaining().value_or(trackCount);


I am confused. Total count and remaining count are not the same.

Estimated = guessed. If unknown we take the number of processed tracks and adjust it when exceeding the maximum.

It took me a while of reading the code to understand but it makes sense. Please add a comment:
"Before starting any tasks, total = remaining"

Thanks for insisting on a clarification. The update during the loop contains a bug. This never happened, because we only iterate over sequences of known size.

src/library/trackmodeliterator.h

Be-ing · 2020-07-04T00:12:58Z

Should this go to the 2.3 branch considering we advised users to clear all keys and reanalyze in the 2.3 beta announcement? I don't care much either way, but it's worth considering.

Be-ing · 2020-07-04T00:14:47Z

@hacksdump could you take a look at this when you get a chance?

Be-ing · 2020-07-04T00:40:21Z

I haven't looked super closely at every detail, but overall this looks like a good improvement. It would be great if we could parallelize these batch operations, but IIUC that isn't possible until we get database transactions out of the GUI thread, right?

katsar0v · 2020-07-04T07:20:23Z

Wanted to test, got this error when tried to compile:

CMake Error at CMakeLists.txt:2206 (message):
  Secure credential storage support requires the Qt5::Keychain component.


-- Configuring incomplete, errors occurred!
See also "/tmp/mixxx/cmake_build/CMakeFiles/CMakeOutput.log".
See also "/tmp/mixxx/cmake_build/CMakeFiles/CMakeError.log".

But it seems to be a useful feature!

Holzhaus · 2020-07-04T11:18:31Z

@katsar0v You need to install qt5keychain or set -DQT_KEYCHAIN=OFF during cmake configuration.

katsar0v · 2020-07-04T12:16:49Z

Yes, I installed it and updated the wiki. I will test again today later and report here

katsar0v · 2020-07-04T14:17:47Z

Ok, I built it. Tested it and it works well.

uklotzde · 2020-07-04T19:34:45Z

The new TaskMonitor abstraction allows to monitor and control multiple worker tasks in the future. Currently it uses QProgressDialog which only works well for a single task.

Added as a proof of concept for a more sophisticated UI solution. It could be extended in the future without affecting existing code.

uklotzde · 2020-07-04T22:46:01Z

Rebased on and retargeted to 2.3 to check if this is feasible.

hacksdump · 2020-07-06T14:05:54Z

but long term it would be better to design a place in the main window for a progress bar. That could also be reused for removing the Analyze library feature and the library scanner popup window.

I can think of something like the progress info in Jetbrains IDEs. It just shows a common status for all global background jobs in the bottom bar.

Holzhaus · 2020-07-06T17:00:00Z

It this window modal because Mixxx should not be used in the meantime? If it was possible to use Mixxx while the jobs are processed, we should make it part of the Notification area I proposed on Zulip: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Notification.20area (not now, I mean long-term).

uklotzde · 2020-07-06T17:17:39Z

It this window modal because Mixxx should not be used in the meantime? If it was possible to use Mixxx while the jobs are processed, we should make it part of the Notification area I proposed on Zulip: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/Notification.20area (not now, I mean long-term).

The modal progress dialog is used because it works out of the box and prevents concurrent UI interactions. Currently those tasks are executed one after another in a simple loop on the UI thread, not as detached jobs. Separate topic, out of scope here.

The task monitoring component is already designed to support concurrent tasks in separate threads with its thread-safe signal/slot interface. It could be displayed in a separate area on the screen. When available the custom progress display of the multi-threaded analysis could be migrated, too.

…u_batchprocessing

Be-ing · 2020-07-10T17:04:45Z

Mixxx still becomes unresponsive with batch jobs, but at least there is some UI indicating what is going on.

src/util/itemiterator.h

Be-ing · 2020-07-10T19:05:23Z

Just some minor comments, but otherwise looks good to me.

Be-ing · 2020-07-10T20:39:21Z

src/library/trackprocessing.cpp

+    // The total count is initialized with the remaining count
+    // before starting the iteration. If the this value is unknown
+    // we use 0 as the default until an estimation is available


Suggested change

// The total count is initialized with the remaining count

// before starting the iteration. If the this value is unknown

// we use 0 as the default until an estimation is available

// The total count is initialized with the remaining count

// before starting the iteration. If this value is unknown

// we use 0 as the default until an estimation is available

Be-ing · 2020-07-10T21:54:58Z

Looks good to me. Waiting on CI.

uklotzde added library ui labels Jun 23, 2020

uklotzde added this to the 2.4.0 milestone Jun 23, 2020

uklotzde mentioned this pull request Jun 23, 2020

[Don't merge / Obsolete] Custom Tags #2656

Closed

11 tasks

uklotzde marked this pull request as draft June 29, 2020 22:23

Be-ing reviewed Jul 3, 2020

View reviewed changes

src/library/trackmodeliterator.h Outdated Show resolved Hide resolved

Be-ing reviewed Jul 3, 2020

View reviewed changes

src/util/itemiterator.h Show resolved Hide resolved

Be-ing reviewed Jul 4, 2020

View reviewed changes

src/library/trackprocessing.cpp Show resolved Hide resolved

Be-ing reviewed Jul 4, 2020

View reviewed changes

src/library/trackmodeliterator.h Outdated Show resolved Hide resolved

uklotzde changed the title ~~[WiP] WTrackMenu: Display a modal progress dialog~~ WTrackMenu: Display a modal progress dialog Jul 4, 2020

uklotzde marked this pull request as ready for review July 4, 2020 22:36

uklotzde changed the base branch from master to 2.3 July 4, 2020 22:40

uklotzde added 4 commits July 5, 2020 00:46

Add framework for batch processing of selected tracks

a1a5a36

WTrackMenu: Show modal progress dialog while processing tracks

697b1f7

Hide QProgressDialog behind generalized TaskMonitor abstraction

67500b4

Address review comments

04305ff

Merge branch '2.3' of git@github.com:mixxxdj/mixxx.git into wtrackmen…

1fdbbfb

…u_batchprocessing

Be-ing reviewed Jul 10, 2020

View reviewed changes

src/util/itemiterator.h Outdated Show resolved Hide resolved

uklotzde added 2 commits July 10, 2020 22:28

Fix update of estimated total track items during processing

ed49044

Implement ItemIterator for indexed Qt collections

0eea708

Be-ing reviewed Jul 10, 2020

View reviewed changes

Fix wording of comment

1a0b20d

Be-ing merged commit 80424d4 into mixxxdj:2.3 Jul 10, 2020

Be-ing mentioned this pull request Jul 11, 2020

[WIP] Rhythm Analyzer #2877

Draft

5 tasks

uklotzde deleted the wtrackmenu_batchprocessing branch August 8, 2020 14:36

daschuer mentioned this pull request Feb 26, 2021

Fix waveform reset #3662

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WTrackMenu: Display a modal progress dialog #2899

WTrackMenu: Display a modal progress dialog #2899

uklotzde commented Jun 23, 2020 •

edited

Loading

Be-ing commented Jul 3, 2020 •

edited

Loading

Be-ing Jul 4, 2020

uklotzde Jul 4, 2020

Be-ing Jul 10, 2020

uklotzde Jul 10, 2020

Be-ing commented Jul 4, 2020

Be-ing commented Jul 4, 2020

Be-ing commented Jul 4, 2020

katsar0v commented Jul 4, 2020

Holzhaus commented Jul 4, 2020

katsar0v commented Jul 4, 2020

katsar0v commented Jul 4, 2020

uklotzde commented Jul 4, 2020 •

edited

Loading

uklotzde commented Jul 4, 2020

hacksdump commented Jul 6, 2020

Holzhaus commented Jul 6, 2020

uklotzde commented Jul 6, 2020

Be-ing commented Jul 10, 2020

Be-ing commented Jul 10, 2020

Be-ing Jul 10, 2020

Be-ing commented Jul 10, 2020

		int estimatedTotalCount =
		pTrackPointerIterator->estimateItemsRemaining().value_or(trackCount);

WTrackMenu: Display a modal progress dialog #2899

WTrackMenu: Display a modal progress dialog #2899

Conversation

uklotzde commented Jun 23, 2020 • edited Loading

Be-ing commented Jul 3, 2020 • edited Loading

Be-ing Jul 4, 2020

Choose a reason for hiding this comment

uklotzde Jul 4, 2020

Choose a reason for hiding this comment

Be-ing Jul 10, 2020

Choose a reason for hiding this comment

uklotzde Jul 10, 2020

Choose a reason for hiding this comment

Be-ing commented Jul 4, 2020

Be-ing commented Jul 4, 2020

Be-ing commented Jul 4, 2020

katsar0v commented Jul 4, 2020

Holzhaus commented Jul 4, 2020

katsar0v commented Jul 4, 2020

katsar0v commented Jul 4, 2020

uklotzde commented Jul 4, 2020 • edited Loading

uklotzde commented Jul 4, 2020

hacksdump commented Jul 6, 2020

Holzhaus commented Jul 6, 2020

uklotzde commented Jul 6, 2020

Be-ing commented Jul 10, 2020

Be-ing commented Jul 10, 2020

Be-ing Jul 10, 2020

Choose a reason for hiding this comment

Be-ing commented Jul 10, 2020

uklotzde commented Jun 23, 2020 •

edited

Loading

Be-ing commented Jul 3, 2020 •

edited

Loading

uklotzde commented Jul 4, 2020 •

edited

Loading