Effects preferences fixes

[ronso0]

https://bugs.launchpad.net/mixxx/+bug/1947921
https://bugs.launchpad.net/mixxx/+bug/1947924
https://bugs.launchpad.net/mixxx/+bug/1947615

[daschuer]

Unfortunately this is still not working correctly. Via left clicks or keyboard, I can still select one effect in each box. The buttons below are no longer enabled depending on the selection.

[daschuer]

Did you consider the idea of the idea of a third tab, so we can follow the normal "dual list buider" pattern for both? In that case, we don't need to fix this unique solution. https://bugs.launchpad.net/mixxx/+bug/1947803

[daschuer]

By the way, thank you for jumping in.

[ronso0]

Unfortunately this is still not working correctly. Via left clicks or keyboard, I can still select one effect in each box. The buttons below are no longer enabled depending on the selection.

Fixed. I can't reproduc with the keyboard at all. Only with the mouse when I

• I select an effect in the Quick Effect list
• click on the empty area below the chains inside in the other list
• then click on an effect there

effects are selected in both lists.
Maybe I can take another look tonight.

Btw the Tab behaviour is confusing right now, someone need to add a proper <tabstops> node to the ui file.

[ronso0]

Also, please test if exporting, deleting etc. still works as expected.

[ronso0]

@daschuer A simper solution is in place, please test.

[daschuer]

Thank you.
I can confirm that https://bugs.launchpad.net/mixxx/+bug/1947921 and https://bugs.launchpad.net/mixxx/+bug/1947924
are fixed now.

I can also confirm that the tab order is broken. Should I file a bug for it?

Since the debug assertion was a rare bug, I am not able to verify if this is fixed as well. Do you have an explanation why it has happened and why it is fixed now?

The import works, but it was surprising that the imported chain appears in the end of both lists my expectation was that It should only appear at the selected position. Can you confirm this?

[ronso0]

Since the debug assertion was a rare bug, I am not able to verify if this is fixed as well. Do you have an explanation why it has happened and why it is fixed now?

This is caused by update() (called by reset or cancel) clearing the list one by one. This would send dataChanged signals if there was an effect selected previously. I added a comment e918be0

[ronso0]

The import works, but it was surprising that the imported chain appears in the end of both lists my expectation was that It should only appear at the selected position. Can you confirm this?

Didn' test. Actually I think it's better to add it to both lists. Then it's available in the fx units and for Quick effects immediatley.
I wouldn't expect it's added only to the focused list. For example when you open the preferences a second time there's one list focused from the previous in the preferences, even though the pevious focus is not necessarily related to import.

[ronso0]

appears in the end of both lists

I find that makes it easier to spot, compared to searching it in the lists after import.
A small UX tweak would be to select the new chain in both lists (and scroll to it) to have a clear signal the chain was imported porperly. Won't happen here.

Though I'd expect the export name to be the display name, instead of "[exported chain name] (duplicate)".
And we the export dialog should always open [mixxx settngs]/effects/, not the home folder. If a user want's to export a chain to share it with others it's easy to navigate to any user directory, but the other way around = navigating to .mixxx or the equivalent in Windows requires more effort, also that may be hidden hidden by default.
I think you already mentioned that issue in original 2618. I'll file a bug.

[Be-ing]

The last two commits have identical commit messages? Should they be squashed?

[Be-ing]

And we the export dialog should always open [mixxx settngs]/effects/, not the home folder.

I disagree. The point of exporting a preset is to share it. It does no good to look in ~/.mixxx/effects to do this; on the contrary, that could confuse users by having them "export" to the place where Mixxx looks for presets.

[Be-ing]

The code changes in this branch look good to me. Please clean up the Git history either by squashing the last two commits or changing the commit message of one. Also please ensure your commit messages fit on one line.

[ronso0]

The last two commits have identical commit messages? Should they be squashed?

They don't, the last (not visible) words are different. But I'll squash them anyway.

[ronso0]

And we the export dialog should always open [mixxx settngs]/effects/, not the home folder.

I disagree. The point of exporting a preset is to share it. It does no good to look in ~/.mixxx/effects to do this; on the contrary, that could confuse users by having them "export" to the place where Mixxx looks for presets.

True. Somehow I mixed up 'export' and 'save', which is wrong since chains are saved when being created... forget about it.

[ronso0]

Ready.

[daschuer]

@Be-ing You have merged this, even though I had issued a review with requested changes. This is not OK and violates our rules.

[ronso0]

@daschuer Can you still reproduce the bug via keyboard?

[daschuer]

No. I assume the issue was that the other list was in Control of the buttons. So the fix here has fixed the keyboard issue as well.

The tab issue is still open.do we have bug for it?

My expectations after import was the behaviour of a text editor. If I paste a text it is inserted at the position of the curser, the selected row in our case.

After you changes here, I think it would work reliable even after reopen.

I can also work with the current solution, I just wonder if anyone has the same expectations.

[Be-ing]

@Be-ing You have merged this, even though I had issued a review with requested changes. This is not OK and violates our rules.

Huh? You commented that the bugs were fixed which I double checked with my own testing.

[daschuer]

There was an open discussion. And even if it has the appearance of being ready, it is not up to a third person to judge that. It is also not part of a good and polite working mode, to merge without a formal OK. Please reconsider it.

[uklotzde]

@Be-ing @daschuer Please don't merge PRs while changes are requested. If you request changes please renew or amend your review frequently to indicate which issues are still unresolved after changes have already been made.

[Be-ing]

I am tired of repeating this discussion. I cannot count how many times it has been repeated to use GitHub reviews appropriately and yet it is still ignored regularly.

Are we going to bicker over process or is someone going to review #4471?

[uklotzde]

If this attitude and tone continuous I will even stop all review activities. Just trying to calm things down.

[Swiftb0y]

@Be-ing whataboutism is not the solution here. You (likely on accident) ignored that daschauer had still requested changes. Please stay polite and offer a solution how the open discussion daschauer had can be resolved after the premature merge instead of nagging for someone to review your PR.

[Be-ing]

@daschuer already commented before merging that the bugs were fixed. I don't understand what more there is to discuss.

[Swiftb0y]

As a general piece of thumb, you should have waited for daniel to formerly dismiss their review or approve the PR before merging (or at least ping him to make sure he didn't forget about it). Those are the rules we all agreed to.