DlgTagFetcher new feedback system #4871

fatihemreyildiz · 2022-07-28T15:39:20Z

As we discussed that on Zulip, we wanted to get rid of the full screen status page in Import metadata from MusicBrainz - DlgTagFetcher.

The new GUI is simply like this:

On startup, progress
After fetching completed, successful
Any case of error, failure:

What do you think generally?

daschuer · 2022-07-28T20:21:27Z

Thank you. This already better for me than the original.

Just looking at the layout the buttons are looking a bit "random" ordered, unfortunately I have not yet a better idea.
Here my points:

The Apply button is not visually connected to the selected line which should be applied.
The Retry button is very prominent visible.
Maybe it helps to gray them out when not in action.

Would it be better to have retry between Previous and Next? Unsure because that break them apart ...
@ronso0 Do you have Ideas?

Other issues:

You can select (highlight) the existing set of metadata, is this necessary?
The progress bar starts with a few small steps and the jumps to 100 %. That can be better scaled.
We can even start it at 10 % and progress form there.
After Retry the bar sticks at 100 %. I have expected that it falls back to 10 % or such

Most of the issues where already part of the existing layout, so it is not necessary required to solve them here.

src/library/dlgtagfetcher.cpp

fatihemreyildiz · 2022-07-28T20:52:34Z

Thank you for the feedback. I'm glad that you liked it.

I have two ideas regarding with the retry button.

The Retry button is very prominent visible.
Maybe it helps to gray them out when not in action.

What I thought at the first place, I also mentioned than on Zulip, we can put an icon instead of the text Retry, with that the button can be little in width and height can stay still. So the button will be square.

Making them gray can be an option, we can also make it invisible by `setVisible' if the fetch is processing it can be invisible in case of failure or success the retry button can appear. So we can get user's attention. What do you think?

The Apply button is not visually connected to the selected line which should be applied.

I didn't get that part, should the apply next to the found metadatas.

You can select (highlight) the existing set of metadata, is this necessary?

It was like this before, I didn't want to change the original version. But yeah, I realize that it is not necessary. I thought it can be useful for the cover art fetcher, but not necessary for this too.

The progress bar starts with a few small steps and the jumps to 100 %. That can be better scaled.

This is happening due to less available metadata. Normally the scaling works related with the number of metadatas. For the less available metadata I think manual scaling can be done.

We can even start it at 10 % and progress form there.

This is an also good approach,

After Retry the bar sticks at 100 %. I have expected that it falls back to 10 % or such.

Oh I didn't notice that until now, Thanks for letting me know.

With the quick tests that I did, this is happening with the same as above, less available number of metadatas. I will fix it asap 👍

src/library/dlgtagfetcher.cpp

daschuer · 2022-07-29T21:40:58Z

I have the case that the dialog is at 100% forever with:
"Fetching track data from the MusicBrainz database"
It looks like an error case is not full handled.

daschuer · 2022-07-29T21:42:58Z

Can you also implement the graying out of Retry and Apply?

daschuer · 2022-07-29T21:46:28Z

The "Fetching track data from the MusicBrainz database" issue remains even after Retry.
I have to reopen the dialog to make it work again.

src/library/dlgtagfetcher.cpp

fatihemreyildiz · 2022-07-30T06:33:37Z

Can you also implement the graying out of Retry and Apply?
Yeah, I can do that.

We can also make Retry button invisible unless if there is a failure. So user can only interact-see it if there is a failure. Non button is better than gray button IMHO. What do you think?

I have the case that the dialog is at 100% forever with:
"Fetching track data from the MusicBrainz database"
It looks like an error case is not full handled.

The "Fetching track data from the MusicBrainz database" issue remains even after Retry.
I have to reopen the dialog to make it work again.

Sometimes some tracks have special cases. Such as the track has only one AcoustID but many MusicBrainz releases or many AcoustID's or just one for each. These can cause different behavior on the status bar.

I haven't encountered these in my tests. It can be because of the tracks that I have in my library, I need to expand my library in order to test it better.

Could you please give me the songs that these errors occurs in your library? So I can have a better perspective.

daschuer · 2022-07-30T06:58:31Z

Non button is better than gray button IMHO. What do you think?

In this case yes. Is there a use case to "retry" even if we have results, probably not.

daschuer · 2022-07-30T07:05:59Z

Could you please give me the songs that these errors occurs in your library?

Today o cannot reproduce the issue, so I cannot identify a specific track. I did not expect that this is a track related issue, more a network responds thing. We need to keep an eye on it.

fatihemreyildiz · 2022-07-30T09:45:38Z

Today o cannot reproduce the issue, so I cannot identify a specific track. I did not expect that this is a track related issue, more a network responds thing. We need to keep an eye on it.

Exactly, we need to keep an eye on it. I have been trying to detect why it is happening. It happened to me once. I added a lot of track into my library to find out. But I couldn't find out why exactly it is happening. All I can say that this is no related with the amount of metadata found as I mentioned earlier.

It actually get results from the MusicBrainz, but It doesn't update the stack. I can see the network result on console it says received network reply after ***ms but not on stack no result displayed. It is not happening for the same track after closing and re-opening or changing the track with the prev-next buttons.

I will work on it more to solve it 👍

fatihemreyildiz · 2022-07-30T16:35:14Z

Thanks for all the feedback :)

I have tried to fix all the errors mentioned above. I think it has a good use case.

I have also made retry button invisible if the track has no metadata available on MusicBrainz database. So with that, I prevent over requests for these tracks. The button will be only displayed if the user lost network connection while fetching, or the track metadata didn't fetched due to 403 (there is still an interesting issue regarding with it) .

ronso0 · 2022-07-30T21:44:14Z

Sorry I'm a bit late to the party..

Would it be better to have retry between Previous and Next? Unsure because that break them apart ...
@ronso0 Do you have Ideas?

First impression when looking at the successful query: the status bar is very prominent (maybe it's just your theme but I doubt it's less prominent in others). Can we show the status bar only when fetching metadata, then replace it with an unobrusive label like "query successful"?

Same for the the Retry button: it is only required in case something goes wrong / gets stuck.
On success it can be ~~hidden~~ greyed out.

ronso0 · 2022-07-30T21:48:14Z

We can also make Retry button invisible unless if there is a failure. So user can only interact-see it if there is a failure. Non button is better than gray button IMHO. What do you think?

IMO it's better to always show all buttons and disable / enable them when necessary.

fatihemreyildiz · 2022-07-31T07:59:53Z

Hey @ronso0 Welcome to the Party! Thanks for the feedback.

I will make all the button visible, and also I will try to change the progress bar to the label if the fetching successfull. I think that this will be better in user case.

I have another question related with the cover art fetcher. It is quite early but i wanted to ask. Maybe we should considwe it after the cover art fetcher PR update after this one.

So my question is, We also want to use the Progress Bar while fetching the cover art thumbnails to inform user about the status. I can not be sure if we should add another progress bar for the cover art fetcher (below the layouts), or should we use this existing one. Would this design look good when we implement the cover art fetcher?

fatihemreyildiz · 2022-07-31T16:08:55Z

With the latest changes Tag Fetcher Dialog looks like this after success.

I have also added 2 launchpad bug related links in order to increase the success of the importing metadata. If we are able to fix those, that will be better, success will be increased and we won't need to inform user for rate-limiting or empty XML.

src/musicbrainz/web/musicbrainzrecordingstask.cpp

daschuer · 2022-08-02T21:54:45Z

This is my view from this branch. Unfortunately the text is cut of.

Did you consider to remove the test on to p of the window?

The progress bar works good. It is only a pity that the dialog is not yes populated on the fly.

fatihemreyildiz · 2022-08-15T19:44:11Z

With the last changes,

I have tried to add QTimer. Now every request done in a second without sleep and hot loops. That was also mentioned on launchpad. Launchpad Bug
I have also tried to solve the bug '404/Empty-XML' mentioned on launchpad
I think now, there is no cut of issue aswell.
Retry button was still was enabled in some situations which shouldn't, that fixed.
At last, namings are changed.

daschuer

I have added some comments

src/library/dlgtagfetcher.cpp

daschuer · 2022-09-18T20:45:38Z

src/library/dlgtagfetcher.cpp

+// The dialog populates with the 0%.
+// After each constant step progress bar value increases by 15%.
+// Before we get recording(s) from AcoustID, the QProgressBar will be 45%.
+// Last step set to 55% and it is going to be divided to recording(s) received from Acoust ID.


Suggested change

// Last step set to 55% and it is going to be divided to recording(s) received from Acoust ID.

// The remaining 55% are divided by the number of recordings received from AcoustID.

This sentence is now redundant with the one above, can you join them?

I've changed the whole comment section, now it looks a bit more cleaner and easier to understand IMHO.

daschuer · 2022-09-18T20:50:59Z

src/library/dlgtagfetcher.cpp

+        // We clear the results to update the Original Tag.
+        // It is listed with the guessedTrackReleases
+        // Without resetting the Data()
+        // Clearing the results and adding them will only update the original tag.
+        // If the track metadata listed in somewhere else such as QLabel.
+        // We wouldn't need to clear the results, only updating the label would be enough.


Suggested change

// We clear the results to update the Original Tag.

// It is listed with the guessedTrackReleases

// Without resetting the Data()

// Clearing the results and adding them will only update the original tag.

// If the track metadata listed in somewhere else such as QLabel.

// We wouldn't need to clear the results, only updating the label would be enough.

// Track has not change: This is a Retry.

The other explanations should be moved closer to the code. To be honest, I don't understand them well. Maybe you can improve them along the way.

Yes 👍 , there will be different scaling for the cover art fetcher as well, so more of these comments will be needed IMHO.

daschuer · 2022-09-18T20:55:37Z

src/library/dlgtagfetcher.cpp

+        addDivider(tr("Original tags"), results);
+        addTrack(trackColumnValues(*m_track), kOriginalTrackIndex, results);
+
+        addDivider(tr("Suggested tags"), results);


the name "result" is hard to understand. I think it is the QTreeWidget showing the results. Can we find a better name for it?

"Tag"s maybe?

src/library/dlgtagfetcher.cpp

daschuer

Some final comments

src/library/dlgtagfetcher.cpp

daschuer · 2022-09-28T07:24:41Z

src/library/dlgtagfetcher.cpp

+        // Track has just updated with the suggested tag.
+        // Original tag needs to be updated as well.
+        tags->clear();
+        addDivider(tr("Original tags"), tags);


This code is redundant with loadTrackInternal

loadTrackInternal is called when the track is changed via buttons Next and Prev and in the first population, IMHO we need it in order to show the actual metadata of the track when the menu is changed.

slotTrackChanged is called when the new metadata applied.

src/library/dlgtagfetcher.cpp

daschuer

LGTM and works good. Thank you.

github-actions bot added the library label Jul 28, 2022

daschuer reviewed Jul 28, 2022

View reviewed changes

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved

daschuer reviewed Jul 28, 2022

View reviewed changes

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved

daschuer reviewed Jul 29, 2022

View reviewed changes

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved

daschuer reviewed Jul 29, 2022

View reviewed changes

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved

daschuer reviewed Aug 1, 2022

View reviewed changes

src/musicbrainz/web/musicbrainzrecordingstask.cpp Outdated Show resolved Hide resolved

uklotzde reviewed Aug 1, 2022

View reviewed changes

src/musicbrainz/web/musicbrainzrecordingstask.cpp Outdated Show resolved Hide resolved

uklotzde reviewed Aug 1, 2022

View reviewed changes

src/musicbrainz/web/musicbrainzrecordingstask.cpp Outdated Show resolved Hide resolved

daschuer reviewed Aug 2, 2022

View reviewed changes

src/musicbrainz/web/musicbrainzrecordingstask.cpp Outdated Show resolved Hide resolved

fatihemreyildiz force-pushed the tagfetcherprogressbar branch from 3260cc1 to 77d0756 Compare August 15, 2022 19:36

fatihemreyildiz mentioned this pull request Aug 17, 2022

Ask user after changing the cover art. #4887

Draft

fatihemreyildiz marked this pull request as ready for review August 19, 2022 21:44

fatihemreyildiz requested review from daschuer and removed request for daschuer August 22, 2022 08:47

fatihemreyildiz added 15 commits September 18, 2022 21:23

dlgtagfetcher: redundant else deleted

fe92769

dlgtagfetcher: fix remaining status message

8f3f195

dlgtagfetcher: retrybtn on only for network errors

787e212

dlgtagfetcher: fix empty xml, retry button.

0371268

dlgtagfetcher: related launchpad links added.

4d6a045

dlgtagfetcher: UI-GUI changed, succes label added

15893d5

dlgtagfetcher: better scale added for progressbar.

8a4a308

dlgtagfetcher: magic numbs deleted constants added

d34a85d

dlgtagfetcher: retry's redundant cycle deleted.

9cda679

dlgtagfetcher: constant value added for index.

349b739

dlgtagfetcher: description added for constants.

3d9b241

dlgtagfetcher: Status prefix deleted.

f0b45ef

dlgtagfetcher: qwarning and error messages added.

f11534d

dlgtagfetcher: wrong constant index fixed.

cc672d4

dlgtagfetcher: dispose stack() & namings changed

2061945

fatihemreyildiz force-pushed the tagfetcherprogressbar branch from 4577ab3 to 2061945 Compare September 18, 2022 18:23

daschuer requested changes Sep 18, 2022

View reviewed changes

dlgtagfetcher: naming & comments polished.

47bbc15

daschuer requested changes Sep 28, 2022

View reviewed changes

fatihemreyildiz added 3 commits September 28, 2022 14:24

dlgtagfetcher: when apply update original tag added

c7001a5

dlgtagfetcher: comments added & warning updated

5182072

dlgtagfetcher: redundant lines deleted

79ce66b

fatihemreyildiz requested a review from daschuer September 28, 2022 12:16

daschuer reviewed Sep 28, 2022

View reviewed changes

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved

daschuer reviewed Sep 28, 2022

View reviewed changes

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved

dlgtagfetcher: p prefix added for pointers

5fdf038

daschuer approved these changes Sep 29, 2022

View reviewed changes

daschuer merged commit 52ed7ee into mixxxdj:main Sep 29, 2022

daschuer added this to the 2.4.0 milestone Sep 29, 2022

daschuer added the changelog This PR should be included in the changelog label Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DlgTagFetcher new feedback system #4871

DlgTagFetcher new feedback system #4871

fatihemreyildiz commented Jul 28, 2022

daschuer commented Jul 28, 2022

fatihemreyildiz commented Jul 28, 2022

daschuer commented Jul 29, 2022

daschuer commented Jul 29, 2022

daschuer commented Jul 29, 2022

fatihemreyildiz commented Jul 30, 2022

daschuer commented Jul 30, 2022

daschuer commented Jul 30, 2022

fatihemreyildiz commented Jul 30, 2022

fatihemreyildiz commented Jul 30, 2022

ronso0 commented Jul 30, 2022 •

edited

Loading

ronso0 commented Jul 30, 2022

fatihemreyildiz commented Jul 31, 2022

fatihemreyildiz commented Jul 31, 2022 •

edited

Loading

daschuer commented Aug 2, 2022

fatihemreyildiz commented Aug 15, 2022

daschuer left a comment

daschuer Sep 18, 2022

fatihemreyildiz Sep 19, 2022

daschuer Sep 18, 2022

fatihemreyildiz Sep 19, 2022

daschuer Sep 18, 2022

fatihemreyildiz Sep 19, 2022

daschuer left a comment

daschuer Sep 28, 2022

fatihemreyildiz Sep 28, 2022 •

edited

Loading

daschuer left a comment

	// Last step set to 55% and it is going to be divided to recording(s) received from Acoust ID.
	// The remaining 55% are divided by the number of recordings received from AcoustID.

DlgTagFetcher new feedback system #4871

DlgTagFetcher new feedback system #4871

Conversation

fatihemreyildiz commented Jul 28, 2022

daschuer commented Jul 28, 2022

fatihemreyildiz commented Jul 28, 2022

daschuer commented Jul 29, 2022

daschuer commented Jul 29, 2022

daschuer commented Jul 29, 2022

fatihemreyildiz commented Jul 30, 2022

daschuer commented Jul 30, 2022

daschuer commented Jul 30, 2022

fatihemreyildiz commented Jul 30, 2022

fatihemreyildiz commented Jul 30, 2022

ronso0 commented Jul 30, 2022 • edited Loading

ronso0 commented Jul 30, 2022

fatihemreyildiz commented Jul 31, 2022

fatihemreyildiz commented Jul 31, 2022 • edited Loading

daschuer commented Aug 2, 2022

fatihemreyildiz commented Aug 15, 2022

daschuer left a comment

Choose a reason for hiding this comment

daschuer Sep 18, 2022

Choose a reason for hiding this comment

fatihemreyildiz Sep 19, 2022

Choose a reason for hiding this comment

daschuer Sep 18, 2022

Choose a reason for hiding this comment

fatihemreyildiz Sep 19, 2022

Choose a reason for hiding this comment

daschuer Sep 18, 2022

Choose a reason for hiding this comment

fatihemreyildiz Sep 19, 2022

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

daschuer Sep 28, 2022

Choose a reason for hiding this comment

fatihemreyildiz Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Jul 30, 2022 •

edited

Loading

fatihemreyildiz commented Jul 31, 2022 •

edited

Loading

fatihemreyildiz Sep 28, 2022 •

edited

Loading