Library: add multi-line editor delegate for comment column

[ronso0]

I'm working with multi-line comments since day one, mostly to store purchase info and notes in lines 2+, sometimes lyrics.
Though it's rather inconvenient editing these inside the table view with the currently used QLineEdit, e.g. line breaks are not printed which makes navigation cumbersome. Going to Track Properties just for minor edits is also no option for me.

This PR adds a proper multi-line editor delegate.
This wasn't exactly plug'n'play, thanks to some Qt quirks and undocumented 'features'.
The editor now adjusts to content dynamically (line count changes), is limited to the table view and shifted vertically if required.
Editing single-line content looks exactly like current QLineEdits.

The table view was not touched, just the editor is now a QPlainTextEdit, subclassed to allow finishing the edit with Return (like QLineEdit) and disable line-wrap for a manageable and predictable view.
UX is also the same as with QLineEdit:

• hit Enter/Return to commit data
• click outside the editor (or do anything else causing a FocusOut event) to close editor and commit data
• hit Escape to close and discard changes
• if the table is scrolled (on purpose or by accident) the editor remains open and is moved with the table, keeps position on further edits after move

TODO

• style in all skins

[ronso0]

This is ready to roll, exactly what I expect from an embedded editor.
Though, I might tweak and polish if required, and rebase.
Let me know when you start reviewing.

[ronso0]

There is one major issue:
right-clicks that are outside the rectangle of the underlying table index don't trigger the context menu but close the editor.
Trying different stuff to figure what's going on, incl. eventFilter, but no luck so far :|

edit fixed.

[ronso0]

Clazy is mad and wants me to start an infinite loop :p
Indeed I want to call the base class' implementation. All other checks pass.

https://github.com/mixxxdj/mixxx/actions/runs/5587227227/jobs/10212290199?pr=11752#step:9:619

Error: /src/library/multilineeditdelegate.cpp:129:5: error:
Maybe you meant to call TableItemDelegate::paint() instead [-Wclazy-skipped-base-method]
    QStyledItemDelegate::paint(pPainter, option, index);
    ^

How to prevent such advice and run clazy dependants?

[daschuer]

MultiLineEditDelegate is a TableItemDelegate is a QStyledItemDelegate

Its this hierarchy reasonable? I think a solution is to split TableItemDelegate into a common base class and a part not used from MultiLineEditDelegate.

Or just disable the warning with

// clazy:exclude=skipped-base-method

[ronso0]

Its this hierarchy reasonable? I think a solution is to split TableItemDelegate into a common base class and a part not used from MultiLineEditDelegate.

I think so. TableItemDelegate does the basic painting and special subclasses handle the editors and do the special painting, even if it is just to apply custom elide (location delegate).
MultiLineEditDelegate is special in another way as it only needs to handle the editor.
I tried to have the least invasive implementation for this new delegate.

Life would be easier if we had a default

void TableItemDelegate::paintItem(painter, option, index) const {
    QStyledItemDelegate::paint(pPainter, option, index);
}

and not force subclasses to implement paintItem(), but override if required.
I'll push a demo commit.

Splitting up TableItemDelegate is possible, sure, though more work and makes maintenance more complicated than necessary I think (a bit like WLibraryTableView and WTrackTableView where things got mixed up IMHO)

PS
Thanks for the hint!

Or just disable the warning with

// clazy:exclude=skipped-base-method

[daschuer]

Looks and works good. Ready for merge?

[ronso0]

Given all the funny Qt 'features' I stumbled upon I'd appreciate manual tests on Win & macOS before 😬
I'm pretty sure I'm not available the week before / during the proposed 2.4 release date, and I'd like to merge this with a good feeling, and not rush afterwards to fix infamous macOS quirks.

from the top of my head: @JoergAtGithub @foss- @m0dB could give this a shot?

To be tested:

• one-line comments should look exactly as before (like other tag string editors), no scrollbars, with one exception:
• comment editor is limited to the viewport (track table rectangle), regardless if the comment column is wider or how many lines are inserted, i.e. if there are scrollbars they should always be on screen
• Shift+Return should insert line break, or let's say Return+modifiers shouldn't close the editor
• Return key without modifiers should finish editing and commit data
• Esc should cancel editing and discard changes
• scrollbars should only kick in if the text is really wider and/or taller (incl. the reserve for scrollbars at the bottom) than the table viewport
• right-click anywhere inside the comment editor should bring up the correct context menu (Copy, Paste, ..), also on scrollbars and the blank space below the last line (Scroll to etc.) not close the editor unexpectedly

[m0dB]

Hi @ronso0, I just did some testing and all points on your "To be tested:" list work fine. But there are some things I notice:

Some alignment/padding issues. I guess the first one falls under "TODO: style in all skins":

• the text in the editor (when single line, or the first line in case of multiple lines) is displaced in relation to the cell text; a couple of pixels to the left and to the top. I notice that with the normal (single-line) textfield it also displaces a little bit (to the left), but significantly less noticeable. With the right padding the text should stay at the exact same position as the text in the cell (and while at it, this should also be fixed for the normal textfield)

But then there is some weirdness with the editor's height that I think goes beyond styling:

• When you hit shift-enter to make a single-line comment multi-line, the editor height becomes much more than needed (more than half a line height). In other words, the bottom padding is much more with multi-line than with single-line (at least visually, there might be no actual padding at play here, just height calculation?)

• Now when you delete the newline and go back to single-line, the editor becomes less high than it was original (!)

And finally some issues with the scrolling / positioning in the viewport. I very much like the solution of limiting/binding the editor to the viewport, but:

• When the comment is the right-most column, it's scrollbar can get obfuscated (partically or totally) by the table view scrollbar.

• I can't scroll the tableview with it's scrollbar while the editor is open (because as soon the editor loses focus it closes) and I would say this is a good thing. But if I scroll the tableview with two-finger swipe (and I guess mouse-wheel scroll behaves the same) I can circumvent this and when the tableview scrolls, the editor moves along, and this breaks how the editor is limited to the viewport. If I do this with my cursor outside the editor, I can scroll the tableview at will, if I do it with my cursor inside the editor, it first scrolls the editor and when it's at it's limit, the scrolling is affecting (but not fully) the tableview.

I think this could be solved by keeping the editor rect clamped to the viewport rect. So if the editor is full height it will stay where it is. Otherwise it will scroll along with the tableview until it's top or bottom reaches the viewport top/bottom. (I guess this would be easier if the editor is not a child of the tableview).

Alternatively:

• Completely block scrolling of the tableview while the editor is open
• Close the editor as soon as the tableview starts scrolling

But as your solution of limiting the editor to the viewport is very elegant and works well otherwise, I think it's worth doing this with the same solution applied to scroll.

[ronso0]

Thanks for testing!

When you hit shift-enter to make a single-line comment multi-line, the editor height becomes much more than needed (more than half a line height)

This is space reserved for the h-scrollbar. QPlainTextEdit would add a bottom padding, too. IIUC this stems from centerOnScroll = true but I couldn't eliminate it with setCenterOnScroll(false) 🤷‍♂️ Also, the padding varied depending on the line count.
I preferred adding a fixed height over checking for scrolbars when calculating the height because that complicates things. If you care to fix that I'd be happy to receive a PR.

[ronso0]

Now when you delete the newline and go back to single-line, the editor becomes less high than it was original (!)

That can be fixed I guess.

[ronso0]

When the comment is the right-most column, it's scrollbar can get obfuscated (partically or totally) by the table view scrollbar.

Oh, macOS' transient/translucent scrollbars... Will see what I can do.
They alway overlap the table content on macOS?

Completely block scrolling of the tableview while the editor is open

Seems pretty straight-forward since it can be done in WTrackTableView and would apply to all editors.
Hm, need to check how it works with cursor moves and WLibraryTableView::moveSelection. And how star rating delegate likes that.

I think closing editors (discard changes, like hitting Esc) on scroll events is better.

[ronso0]

Now when you delete the newline and go back to single-line, the editor becomes less high than it was original (!)

That can be fixed I guess.

Done.

@m0dB Please try again.
The editor is shrunk if it may interfere with the scrollbars.
Minimum height is always that of the respective table cell (initial painter rectangle).

[m0dB]

Yes, all good! I would say though that the width is off by one pixel:

See what I mean? You see a tiny strip of the green bar below. And at some point I saw also white pixels from the letters there.

And not sure if this is new, but this happens when there is only one line and it is very long:

With more than 1 line the scrollbar in the space at the bottom, as expected.

[ronso0]

And not sure if this is new, but this happens when there is only one line and it is very long:

This is the default behaviour of QLineEdit and I implemented that for consistency with the other fields.
It shouldn't be cut off though.

FWIW I just managed to get scrollbars for a one-liner so something isn't working entirely as expected.

And I need to polish exit-on-scroll: when scrolling the editor it may happen that scroll events are sent to the table and close the editor unexpectedly. Explicit commands like [Playlist], SelectTrackKnob though should override that IMO. A bit tricky..

[ronso0]

the text in the editor (when single line, or the first line in case of multiple lines) is displaced in relation to the cell text; a couple of pixels to the left and to the top. I notice that with the normal (single-line) textfield it also displaces a little bit (to the left), but significantly less noticeable.

Well, this is because the default QLineEdit implementation uses Qt::AlignVCenter.
To get the same result I could reimplement the paint method (uuh, not really), so I tried a few other ways:

1. setStyleSheet( /* only padding-top */): screws up the font (size) after first edit
2. editor->document()->setDocumentMargin(offset): is applied in all directions, so the text is centered vertically but also shifted horizontally
3. apply the top-margin to the document's first block or to it's rootFrame(): clean solution, though possible only with QTextEdit. Using that however requires a bunch of annoying workarounds and overrides which made this little helper so complex. And it feels like every workaround breaks something else. Dropped that. No fun.

So, 2. works but since it'sapplied in all direction the left shift may feel strange with a low font / row height ratio. Negligible.

Please test again!

[ronso0]

@daschuer I'm sorry, was rebasing onto the first commit (I thought) but rebased onto main which makes the diff rather large. My bad.
All commits built, though you probably want to take another look.

[ronso0]

Okay, I'm happy with this. Hopefully it's fine on macOS, too. Some testing on Windows is required before merging, though I only expect minor quirks if any. @JoergAtGithub maybe?

I can do fixes if required when I'm back from holiday in a few weeks. If there's a chance this slips in before 2.4.0 feel free to push fixup commits yourself.

edit: nope, no styles for skins other than LateNight PaleMoon, yet. already done

[daschuer]

LGTM and works good. Thank you for all the size fideling.

[ronso0]

Yeah, I'm glad it works now. It's not the small adjustments that were annoying but rather same strange quirks / feature. I hope it looks good on Windows, too.