Tracktable editing #1561

daschuer · 2018-03-25T10:05:23Z

This should improve the issues discussed here:
#1550

By an extra checkbox "Edit metadata"

Be-ing · 2018-03-25T10:27:03Z

I don't understand how this helps. QAbstractItemView::SelectedClicked is mutually exclusive with double click actions.

foss- · 2018-03-25T11:30:32Z

This link is a video showing single and double click behavior on macOS 10.13.3. It shows 1x single click , 2x double click, 1x single click. Link valid for 30 days.
macOS waits for the second click before triggering the single click action.

For mixxx library behavior, my current preference would be to allow for a single click to trigger the edit track option (currently broken) and a double click to load the track into deck (working). Additionally have an option to disable editing track data in Preferences > Library (should it even default to off?).

Editing track data is dangerous because

it can happen by accident (and it did happen by accident, since mixxx was unable to differentiate between single and double click properly, resulting in double clicks triggering two options from what I understood). this for one is buggy behavior but it's also dangerous because edits can happen by accident very easily. since mixxx now allows writing metadata back to files, this is especially a serious problem - we don't want users to end up with a library full of tracks called "e" "gh" "c" etc) - see LP1743238
on macOS there is a bug where the waveform becomes unusable whenever edit track was used - see LP1665583

Hope this helps finding a good solution for this little situation.

Be-ing · 2018-03-25T11:48:32Z

Thank you @foss-.

macOS waits for the second click before triggering the single click action.

I think this may be possible to implement this, but I think it would require not using QAbstractItemView::SelectedClicked. But I am doubtful we should rush such a hack.

ywwg · 2018-03-25T14:56:24Z

I think this may be possible to implement this, but I think it would require not using QAbstractItemView::SelectedClicked. But I am doubtful we should rush such a hack.

I am confused by your comments because the behavior @foss- describes is my experience of how mixxx was working before #1550. I could doubleclick to load a track, or click twice to edit the metadata. I never had a problem with double-click triggering metadata editing (in either linux or windows). Is this a problem that only appeared in Mac?

My current suggestion is we revert #1550 and instead add a checkbox in the prefs to disable one-click track editing.

ywwg · 2018-03-25T14:57:05Z

@daschuer I get a build error on this code:

src/preferences/dialog/dlgpreflibrary.cpp: In member function ‘virtual void DlgPrefLibrary::slotUpdate()’:
src/preferences/dialog/dlgpreflibrary.cpp:178:13: error: ‘radioButton_dbclick_edit_metadata’ was not declared in this scope
             radioButton_dbclick_edit_metadata->setChecked(true);
             ^
src/preferences/dialog/dlgpreflibrary.cpp: In member function ‘virtual void DlgPrefLibrary::slotApply()’:
src/preferences/dialog/dlgpreflibrary.cpp:309:16: error: ‘radioButton_dbclick_edit_metadata’ was not declared in this scope
     } else if (radioButton_dbclick_edit_metadata->isChecked()) {

daschuer · 2018-03-25T15:06:11Z

My current suggestion is we revert #1550 and instead add a checkbox in the prefs to disable one-click track editing.

That is what I am about to do. Sorry for the build error.

ywwg · 2018-03-25T15:14:11Z

For the record, if there is some bug where double-clicking is getting interpreted as click-on-selected and metadata editing is getting triggered, that's definitely a bug we should investigate. What I'm confused by is I've never seen that before so I need more information about how the bug gets triggered (OS, double click speed settings, etc?)

daschuer · 2018-03-25T15:20:42Z

There is an issue only by design. The double click recognition works, but if you click too slow on a selected track you are in edit mode. This can most likely happen with a touch pad and the tab click is activated.

…d to deck feature

daschuer · 2018-03-26T21:23:21Z

Puh, that was more work than thought on the first sight. Ready for review.

Be-ing

This works as described. I still think it is a bad idea to have both a SelectedClicked action and a double click action. But as long as I have the option to turn off the SelectedClick action, that's good enough for me.

Be-ing · 2018-03-27T03:21:28Z

src/preferences/dialog/dlgpreflibrarydlg.ui

+      <item row="4" column="0">
+       <widget class="QCheckBox" name="checkBoxEditMetadata">
+        <property name="text">
+         <string>Edit metadata</string>


This string should have more context. How about "Edit metadata after clicking selected track"?

For 2.1, I would like to keep this text, because we already have translations for it.
It is not too bad and details can go to a Tooltip

This enables instant metadata editing within the track table. Editing starts when clicking on an already selected item.

The recently added string has only been translated into DE. You can easily change it again.

OK thank you.

Be-ing · 2018-03-27T03:22:16Z

src/preferences/dialog/dlgpreflibrary.cpp

    default:
            radioButton_dbclick_deck->setChecked(true);
            break;
    }

+    checkBoxEditMetadata->setChecked(
+            m_pConfig->getValue(
+                        ConfigKey("[Library]","EditMetadata"), true));


IMO this is much too dangerous to be enabled by default.

Since this was the default in 2.0 I like to keep it. Windows and Mac user are used to that "feature"

I don't think either of those are good reasons to make this on by default. This behavior is so unexpected and confusing that no one could even describe quite what the problem was until I started working on the code and reading the Qt documentation. If users have to read the "Edit metadata after clicking selected track" description in the preferences before enabling this behavior, I think they will have an easier time understanding how it works and therefore understand how to use it intentionally and avoid activating it unintentionally.

For my feeling is that this topic has stirred up, because of a single users bug report.
All Windows an Mac users expect the feature and can handle it in their file manager.
This feature was enabled for ages in Mixxx, without anyone else complaining.
But we had complains during the short phase with the double click edit, loosing this feature.

So I assume that many user would feel the disabled feature as a regression. It might took a while to discover the check-box to re-enable it.

I do not use this feature and I also have never accidentally enabled it. So I do not care personally about the default.

How do others think?

@daschuer are you referring to https://bugs.launchpad.net/mixxx/+bug/1665583 ? If so, let me reassure you, this has been giving mac users a hard time using mixxx to the point where live usage becomes questionable due to unusable waveforms when accidentally getting into track edit mode. The bug was filed feb 2017 and only disabling edit mode / adding an option in prefs to disable it, makes mixxxx usable for macOS users with trackpad. Let me know if I am missing something. My experience is limited to macOS, so this may be working great on linux / win.

This sounds like a separate issue that needs to be addressed on that platform. Disabling click-to-edit would just be sweeping the problem under the carpet.

Well we have no macOS developers and we are try to get a release out imminently. This is the time for quick hacks, not debugging complicated issues and doing big refactoring.

I'm aware that the bug exists, but it was reported in January of this year and we've had this feature for many years, so this seems to be a new problem on OSX only. I suspect something changed on that platform, like the addition of "force clicks" which is VERY confusing for users who don't know what it is (including myself). Maybe "force click" is enabling edit mode?

What about making the default off for OSX only?

The UI lagging with editing metadata from the track table is only on macOS, but the high risk of accidentally editing metadata is for all OSs. This has been a problem for me on GNU/Linux with Mixxx for as long as I've been using it. I didn't report a bug for it earlier because the behavior is so unintuitive that I couldn't figure out exactly what triggered the accidental edits.

Having different defaults for different platforms doesn't seem like a great idea to me.

Fair enough. My preference is to leave it on by default but as long as there's adequate documentation I'm ok with the default being either way

If the default is changed to off, I think the new inverted "Prevent editing of metadata by clicking selected track" wording is odd.

Be-ing · 2018-03-27T03:33:22Z

src/library/library.cpp

@@ -172,6 +172,9 @@ Library::Library(
    } else {
        m_trackTableFont = QApplication::font();
    }
+
+    m_editMetadata = m_pConfig->getValue(
+            ConfigKey(kConfigGroup, "EditMetadata"), true);


The names in the code should have more context as well as the string shown to the user in the preferences. How about m_editMetadataSelectedClick for the boolean and EditMetadataSelectedClick for the ConfigKey name?

Be-ing · 2018-03-27T03:33:54Z

src/library/library.cpp

    m_iTrackTableRowHeight = rowHeight;
    emit(setTrackTableRowHeight(rowHeight));
 }

+void Library::setEditMedatata(bool enabled) {


setEditMetadataSelectedClick

Be-ing · 2018-03-27T03:35:13Z

src/preferences/dialog/dlgpreflibrary.cpp

@@ -143,6 +139,7 @@ void DlgPrefLibrary::slotResetToDefaults() {
    checkBox_show_itunes->setChecked(true);
    checkBox_show_traktor->setChecked(true);
    radioButton_dbclick_bottom->setChecked(false);
+    checkBoxEditMetadata->setChecked(true);


checkBoxEditMetadataSelectedClicked

daschuer · 2018-03-27T19:06:28Z

Done.

Be-ing · 2018-03-27T19:38:37Z

src/preferences/dialog/dlgpreflibrarydlg.ui

-      <item row="4" column="0">
-       <widget class="QCheckBox" name="checkBoxEditMetadata">
+      <item row="4" column="0" colspan="2">
+       <widget class="QCheckBox" name="checkBoxEditMetadataSelectedClicked">
        <property name="toolTip">
         <string>This enables instant metadata editing within the track table.
 Editing starts when clicking on an already selected item.</string>


I think this tooltip is unnecessary with the updated string.

daschuer · 2018-03-27T21:41:26Z

Ready

Be-ing · 2018-03-27T22:15:45Z

LGTM except for the option being enabled by default.

ywwg · 2018-03-28T14:08:49Z

I'll check this tonight

ywwg · 2018-03-28T14:22:14Z

src/preferences/dialog/dlgpreflibrarydlg.ui

+      <item row="4" column="0" colspan="2">
+       <widget class="QCheckBox" name="checkBoxEditMetadataSelectedClicked">
+        <property name="text">
+         <string>Edit metadata after clicking selected track</string>


How about: "Allow editing of metadata by clicking selected track"

I wonder if this is a good candidate for an inverted bool, because the major use-case is "locking" the library so it can't accidentally be edited: "Prevent editing of metadata by clicking selected track"

Yes, good idea.

ywwg · 2018-03-28T14:23:48Z

src/library/dlganalysis.cpp

@@ -62,6 +62,13 @@ DlgAnalysis::DlgAnalysis(QWidget* parent,
            SIGNAL(selectionChanged(const QItemSelection &, const QItemSelection&)),
            this,
            SLOT(tableSelectionChanged(const QItemSelection &, const QItemSelection&)));
+
+    connect(pLibrary, SIGNAL(setTrackTableFont(QFont)),


is this an unrelated change? I still don't see the correct font being used in the analyze view of the library. (That's been broken for a long time)

ywwg · 2018-03-28T14:25:18Z

The changes works as described and I like this solution a lot, thanks! I just want to make sure the preference text is right before giving LGTM

daschuer · 2018-03-28T20:33:19Z

OK, checkbox inverted and analysis feature connections repaired.

ywwg · 2018-03-29T13:34:24Z

LGTM except for the option being enabled by default.

It is my dream that Mixxx can learn to decide issues of opinion through a consensus process instead of a battle of wills. It's very stressful when some people are saying "this is what I prefer let's find a solution we can all live with" and other people are saying "I refuse to LGTM unless it's done this way."

daschuer · 2018-03-29T18:37:58Z

OK, lets merge this, to have the chance to get the string translated.
The default question can be postponed. Currently, we merge just the 2.0 state which is not too bad.

Be-ing · 2018-03-29T18:41:22Z

I'm confused. I thought we had just reached a consensus to change the default to off?

daschuer · 2018-03-29T19:17:54Z

The checkbox logic is now inverted, so it is off by default and selected click enabled.
This means the behavior does not change during upgrade from 2.0, which is a good idea.

I am a bit confused about the related MacOS GUI issue. It looks like we are hunting a 2.0 bug reading
https://bugs.launchpad.net/mixxx/+bug/1759433
Do we have hints that the GUI lag after library editing is a new issue? If not, I think we can go do a release tomorrow.

Be-ing · 2018-03-29T21:41:08Z

I meant having the selected-click-to-edit behavior off by default. Once again you have prematurely merged your own PR against my objections. This feels really disrespectful.

Do we have hints that the GUI lag after library editing is a new issue?

This is not a new issue. @foss- has been asking about it for a long time -- maybe a year now?

daschuer · 2018-03-29T22:21:25Z

I did not meant to treat you disrespectful, I am sorry for this. We already had an agreement that this PR is ready for merge, except the default question. The default can be changed at any time, no reason to delay this and have an untranslated string for this feature.

This can be treated as a workaround for https://bugs.launchpad.net/mixxx/+bug/1665583, with any default value. But this bug discovers an other strong issue in our Mixxx project in general:
If we are not able to investigate this party stopping bug since 2017-02-17, how could we do this now in reasonable time before the scheduled release. We have simply no one, who is able to reproduce it and fix this issue. Is is questionable if we are able to support Mac OS any longer.
Maybe we should drop the official Mac Support until we find an engaged contributor.

Be-ing · 2018-03-29T23:58:55Z

"LGTM except" does not mean "LGTM". The default is important, especially considering the consequences of this.

Maybe we should drop the official Mac Support until we find an engaged contributor.

I was thinking about that too, but I think that is the opposite of what we should do. I think if we do that, then there's a decent chance we'll never have a Mac contributor again. We have had several Mac developers express interest in contributing lately but all of them have been discouraged by out-of-date and incomplete documentation. I will try to borrow a Mac tomorrow to look into the macOS issues.

daschuer · 2018-03-30T00:03:10Z

Thank you very much. I hope that we see clear after your investigations.

foss- · 2018-03-30T00:11:59Z

Dropping macOS support will send wrong signals and minimize the chance for macOS devs to join in. I've been using mixxx and closely observing it's development for a while now. It's made amazing progress. There are a few serious macOS related bugs. But regardless of the default for this, adding this option will make mixxx a lot more usable on macOS.

So while there are some intense debates going on currently the small mixxx dev team has a lot on their plate to handle and I think the team is doing an amazing job.

Thank you all for your work and dedicating your time to this amazing software <3

poelzi · 2018-03-30T13:45:07Z

To give my 2cents. Whenever I have the edit functionality open, it's by accident. Mixxx is not a good tag management software and does not have to be in my opinion. It's nice if you can fix som mistakes you find while using it, but having such a easy accessible edit functionality is more annoying then it helps.
I'm very happy that I can deactivate it soon.

Added an extra option for library metadata editing

dbbfa25

daschuer changed the base branch from master to 2.1 March 25, 2018 10:05

daschuer added this to the 2.1.0 milestone Mar 25, 2018

daschuer mentioned this pull request Mar 25, 2018

prevent accidental editing of metadata from track table #1550

Merged

Added "Edit Metadata" preferences function, replacing the related Loa…

332ceb6

…d to deck feature

daschuer changed the title ~~[Wip] Tracktable editing~~ Tracktable editing Mar 26, 2018

Be-ing suggested changes Mar 27, 2018

View reviewed changes

daschuer added 2 commits March 27, 2018 20:40

Added tooltips for the "Edit Metadata" option

5e5d092

Impoved variable names

ce8cf63

Be-ing reviewed Mar 27, 2018

View reviewed changes

remove tooltip

7d28e27

ywwg reviewed Mar 28, 2018

View reviewed changes

daschuer added 2 commits March 28, 2018 21:25

Invert checkbox logic

5b872ce

fix connections for dialog preferences to analysis feature.

6dea917

daschuer merged commit 42646db into mixxxdj:2.1 Mar 29, 2018

Be-ing mentioned this pull request Mar 29, 2018

disable track table editing after clicking selected track by default on macOS #1568

Merged

daschuer deleted the tracktable_editing branch September 26, 2021 17:35

mixxxbot mentioned this pull request Aug 23, 2022

Severe UI lockups under OS X #9201

Closed

Tracktable editing #1561

Tracktable editing #1561

Conversation

daschuer commented Mar 25, 2018

Be-ing commented Mar 25, 2018

foss- commented Mar 25, 2018 • edited Loading

Be-ing commented Mar 25, 2018 • edited Loading

ywwg commented Mar 25, 2018 • edited Loading

ywwg commented Mar 25, 2018

daschuer commented Mar 25, 2018

ywwg commented Mar 25, 2018

daschuer commented Mar 25, 2018

daschuer commented Mar 26, 2018

Be-ing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

esbrandt Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

foss- Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Be-ing Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Mar 27, 2018

Choose a reason for hiding this comment

daschuer commented Mar 27, 2018

Be-ing commented Mar 27, 2018

ywwg commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywwg commented Mar 28, 2018

daschuer commented Mar 28, 2018

ywwg commented Mar 29, 2018

daschuer commented Mar 29, 2018

Be-ing commented Mar 29, 2018

daschuer commented Mar 29, 2018

Be-ing commented Mar 29, 2018 • edited Loading

daschuer commented Mar 29, 2018

Be-ing commented Mar 29, 2018

daschuer commented Mar 30, 2018

foss- commented Mar 30, 2018

poelzi commented Mar 30, 2018

foss- commented Mar 25, 2018 •

edited

Loading

Be-ing commented Mar 25, 2018 •

edited

Loading

ywwg commented Mar 25, 2018 •

edited

Loading

daschuer Mar 27, 2018 •

edited

Loading

esbrandt Mar 27, 2018 •

edited

Loading

daschuer Mar 27, 2018 •

edited

Loading

foss- Mar 27, 2018 •

edited

Loading

Be-ing Mar 28, 2018 •

edited

Loading

Be-ing commented Mar 29, 2018 •

edited

Loading