Bug 143114 - Libclplug: kf5 support errors
Summary: Libclplug: kf5 support errors
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: UI (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All Linux (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.3.0
Keywords:
Depends on:
Blocks: KDE, KF5
  Show dependency treegraph
 
Reported: 2021-06-29 12:45 UTC by Armin Le Grand (allotropia)
Modified: 2024-07-10 06:26 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
CalcDoc with DropDown fiels to demonstrate errors (15.57 KB, application/vnd.oasis.opendocument.spreadsheet)
2021-06-29 12:45 UTC, Armin Le Grand (allotropia)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Armin Le Grand (allotropia) 2021-06-29 12:45:10 UTC
Created attachment 173266 [details]
CalcDoc with DropDown fiels to demonstrate errors

The UI-Vcl-plugin lib instdir/program/libvclplug_kf5lo.so has still quite some errors. To use, build with flags creating this lib (--enable-kf5). Start LO using
   "SAL_USE_VCLPLUGIN=kf5 instdir/program/soffice"
in commandline (or set SAL_USE_VCLPLUGIN as usual).

Errors seen (probably not complete list):
To reproduce, open bugdoc (LO528_Filter+Tesla.ods), Use dropdown button at 1st field to open DropDown/PopUp then

(1) Problems with using TAB key, no crash, but main win makes EMPTY REPAINT and stays that way. Needs handish action to show UI && EditView again at all (e.g. resize)

(2) No error with clicking at object selection CheckBoxes, but action is on MouseDown instead of MouseUp, also on leaving CheckBox area with still pressed MouseButton -> not standard, even the other 'normal' buttons trigger on MouseUp (trigger on LeaveArea is BAD since often used by user to terminate unwanted data entry without change)

(3) DropDown/PopUp stays open and in FG when changing app on Linux desktop (keyboard, ALT+TAB) -> very strange, unwanted effects -> e.g. gtk3_kde5 avoids that by simply FORBIDDING app/focus change wile popup is open
Comment 1 Michael Weghorn 2021-07-13 13:26:11 UTC
(In reply to Armin Le Grand from comment #0)
> Errors seen (probably not complete list):
> To reproduce, open bugdoc (LO528_Filter+Tesla.ods), Use dropdown button at
> 1st field to open DropDown/PopUp then
> 
> (1) Problems with using TAB key, no crash, but main win makes EMPTY REPAINT
> and stays that way. Needs handish action to show UI && EditView again at all
> (e.g. resize)

This sounds like the same issue issue as bug 143114, which I encountered just today. I just double-checked it's a regression from the same commit.

> (2) No error with clicking at object selection CheckBoxes, but action is on
> MouseDown instead of MouseUp, also on leaving CheckBox area with still
> pressed MouseButton -> not standard, even the other 'normal' buttons trigger
> on MouseUp (trigger on LeaveArea is BAD since often used by user to
> terminate unwanted data entry without change)

That behaves the same with gen VCL plugin and on Win, so might not be specific to qt5/kf5, but an issue with the general VCL implementation (as opposed to gtk3, which might use native controls)?

> (3) DropDown/PopUp stays open and in FG when changing app on Linux desktop
> (keyboard, ALT+TAB) -> very strange, unwanted effects -> e.g. gtk3_kde5
> avoids that by simply FORBIDDING app/focus change wile popup is open

Same with "gen" in my tests, while gtk3 behaves better.

Can we see (1) as a duplicate of bug 143114?
Are (2) and (3) the same thing or does it possibly make sense to open separate bug reports for those? (unless you plan to fix them anyway ;-))
Comment 2 Michael Weghorn 2021-07-20 06:26:12 UTC
(In reply to Michael Weghorn from comment #1)
> (In reply to Armin Le Grand from comment #0)
> > Errors seen (probably not complete list):
> > To reproduce, open bugdoc (LO528_Filter+Tesla.ods), Use dropdown button at
> > 1st field to open DropDown/PopUp then
> > 
> > (1) Problems with using TAB key, no crash, but main win makes EMPTY REPAINT
> > and stays that way. Needs handish action to show UI && EditView again at all
> > (e.g. resize)
> 
> This sounds like the same issue issue as bug 143114, which I encountered
> just today. I just double-checked it's a regression from the same commit.

Jan-Marek's pending fix for tdf#143334 pending at https://gerrit.libreoffice.org/c/core/+/119237 actually fixes this aspect for me.
Comment 3 Jan-Marek Glogowski 2021-07-23 19:34:54 UTC
(1) should be fixed by the fix for tdf#143334
(3) should be fixed by commit 63f92f185b78db5a575da58efac3f94a8cb5a5f6 ("Qt5 fix Qt::Popup window handling")

(2) is ... complex. It's not a normal checkbox problem. These work correct. The problem is a checkbox inside a treeview.

If you have a treeview, you're actually handling rows. And if you hold the left button and move the mouse, you start a drag operation of the row.

void SvTreeListBox::StartDrag( sal_Int8, const Point& rPosPixel )
{
    Point aEventPos( rPosPixel );
    MouseEvent aMouseEvt( aEventPos, 1, MouseEventModifiers::SELECT, MOUSE_LEFT );
    MouseButtonUp( aMouseEvt );

So now you know that actually when starting a drag you don't, because LO now generates a MouseButtonUp + MouseEventModifiers::SELECT event...

And select means toggle for a checkbox, which is what you observe as wrong, when you start the drag. Then there is a lot of special handling code, when you actually start the drag operation on a checkbox (AKA SvLBoxItemType::Button), see SvImpLBox::ButtonDownCheckCtrl and friends.

If you comment these three lines, you get a selection following your mouse. Maybe that is actually the correct behavior for a normal list view? AKA you just select on mouse up (but just if you don't start the drag by clicking on a button currently). But then it still toggles the checkbox for the last selected row, even when you release the mouse outside the widget. And what should happen, if you start the select on a row, but release on a 2nd? If you follow the "selection on up", that should select the current row.

And it gets more interesting if you have rows with multiple buttons, like in Options >> Load/Save >> MS Office. There toggle on selection doesn't make any sense IMHO...

P.S. and there was tdf#116675, the parent for a whole lot of regression and other "pleasantries", like https://gerrit.libreoffice.org/c/core/+/108947 (and instead of implementing a click handler for the autofilter list box, NIST changed the general widget...). Instead of having a special handler for row selection, which toggles the checkbox, we get a more complex general widget...

Current conclusion: any fix on top of this can get messy with potential regressions.
Comment 4 Jan-Marek Glogowski 2021-07-23 19:47:54 UTC
My suggestion is to test (1) and (3), then close this and open a new report for (2). And we need something that defines the expected behavior, before we can even start implementing a "fix".
Comment 5 Armin Le Grand (allotropia) 2021-07-26 08:39:35 UTC
Looking for (2). As always, it's more complicated as thought, e.g. in standard mode gtk3 I checked for comparison a normal (as I thought) CheckBox in Tools/Options/UserData, e.g. "Use Data for Document Prioperties". That one can be interrupted in action after MouseDown to the left, top and bottom, but NOT to the right (?) - VERY strange. So the user who wants to interrupt his false-click has to be lucky to go to the correct direction to do so - also NO optical feedback while executing - that is usual to lead/support the user in what he is doing :-(

This might hint to some weird leaving-follow-mousemovement area (despite MouseFollow should be active until MouseButtonUp). In that case the non-conform behaviour in the CheckBox in the DropDown may have to do with being restricted to all sides (?)

And this is in standard gtk3 mode. Wanted to look at kf5-mode, though...
Comment 6 Armin Le Grand (allotropia) 2021-07-26 09:15:18 UTC
@Jan-Marek: Yes, SvTreeListBox::StartDrag gets the MouseButtonDown. In principle it's not complicated. If a TreeListEntry has a CheckBox and it gets hit, do NOT start the Drag as here, but forward that action to the CheckBox. I know, sounds simple, but is probably not. It depends if a ListBoxEntry has a 'real' CheckBox or only 'fakes' it. If it's a real one, action can be forwarded. If not, it would also have to be 'faked'. Pobably the rest of actions would also happen at SvTreeListBox, so - until action is finished - some 'forward' mode would be needed (or faked).
In short: The hierarchy represented by SvTreeListEntry when containing active UI elements like CheckBoxes (seen in the code there might also be buttons?) HAS to be used/represented on MouseActions by checking if these are hit && forwarding the action.
That would be the only clean solution, maybe including a 'forward' mode while mouse is captured (in this case by CheckBox). Need to find out if there exists a real CheckBox. Faking all that if not would mean to double the CheckBox behaviour locally - not effective and error-prone as soon as someone changes the CheckBox behaviour.
Problem is this cannot really be debugged - argh - since this immedialely leads to deadlocks in the UI using gdb in any form in another window due to the mouse being locked :-(
Comment 7 Armin Le Grand (allotropia) 2021-07-26 09:36:39 UTC
It's an SvLBoxButton which is capable to show a CheckBox && a Button - sigh. In different UIs it might still be incarnated as 'real' Window/Button, so the question stays why it does not offer the normal UI's functionality to end a MouseButtonDown by leaving the area for MuseButtonUp...?
Comment 8 Armin Le Grand (allotropia) 2021-07-26 15:51:19 UTC
SvTreeListBox just has a list of ListBoxEntry(s) and these have SvLBoxButton(s) describing CheckBoxes/Buttons.
I would say that MouseActions may/should/haveTo handle these if e.g. the CheckBox needs to be switched (it may work now due to 'something' doing this or being a real 'Window' in the resp. UI?).
In consequence these would have to
- check if SvLBoxButton is hit
- start mouseLock, switch
- during MouseMoves, may change state (pressed or not when outside)
- at MouseButtonUp is the point to end mouseLock && execute action, NOT before
Not sure how far that stuff interferes with the UI integrations of Qt and ssimilar stuff, but that's what *should* happen. I see that it's more easy to just execute (and then try to drag the ListEntry, BTW not needed when Button is hit)..?
Comment 9 Armin Le Grand (allotropia) 2021-07-28 15:12:15 UTC
Debugging this is not really possible, so I try to understand the code - SvTreeListBox is a nightmare, with it's base classes && massive implementations. I also used debug to see that the SvTreeListBox::StartDrag gets triggered by some Qt action, while e.g. MouseButtonDown is triggered regular by the office.
Also we have Win/Linux/Mac. I just checked Linux where we have five UI possibilities (currently) selected by e.g. setting env SAL_USE_VCLPLUGIN:
(a) "gen"
(b) "gtk3_kde5"
(c) "gtk3"
(d) "qt5"
(e) "kf5"
To get all of these to experimant you need to build using
--enable-kf5
--enable-qt5
--enable-gtk3-kde5
in autogen configuration.
Versions (a..e) behave not the same, so the 1st problem would be to make them behave the same. BTW: A good example how a CheckButton *should* behave is in the BugDoc right above those lists when shown, the 'All' button - which works the same in (a..e).

Checking why SvTreeListBox::StartDrag trggers a MouseButtonUp-event - maybe there are hints in the git history. Interestingly this is triggered by MouseMove, *not* from MouseButtonDown/Up (!)...
Comment 10 Armin Le Grand (allotropia) 2021-07-28 15:24:47 UTC
The three lines from SvTreeListBox::StartDrag originate from ac7acb0ab1329913b0cec79790adcde0263960be "Merged SvTreeListBox and SvLBox" from 2012-10-11 - pretty old. Thus it originates from "void SvLBox::StartDrag" and was merged-in to SvTreeListBox::StartDrag. No idea why it was like that in SvLBox, but it was *not* originally part of SvTreeListBox. This is like that since 2021, so I will not dig deeper - those three lines may not be needed, but no direct hints from the past :-(

Basic problem stays that SvTreeListBox::StartDrag (triggered from a MouseMove, NOT from MouseButton) *always* executes that MouseButtonUp. This should not happen if SvImpLBox (the EntryList) is actively handling a ButtonAction with CaptureMouse() being active.
That again is alredady done actively in SvImpLBox::ButtonDownCheckCtrl, SvImpLBox::MouseMoveCheckCtrl, SvImpLBox::ButtonUpCheckCtrl. Adding a possibility to *ask/check* for it from SvTreeListBox...
Comment 11 Armin Le Grand (allotropia) 2021-07-28 17:11:52 UTC
Really hard to debug/change stuff here, but I made some progress and using kf5 it's already pretty good. Thus I created https://gerrit.libreoffice.org/c/core/+/119644 as a suggestion/first step to enhance this situation.
As described there, the behavior is also dependent on the UI implementation used
under Linux (gen/gtk3_kde5/gtk3/qt5/kf5) which are all (unfortunately) behaving differently... :-(
Comment 12 Armin Le Grand (allotropia) 2021-07-29 15:18:14 UTC
Checking linux render backends (a..e):
(a) "gen": works as expected. Exception: Start klick in text, move to other entry, MouseButtonUp switches other entry. This should not trigger/change anyting if start and end entry are different. Leaving box completely works as expected.
(b) "gtk3_kde5": Switches on MouseDown
(c) "gtk3": same as (b)
(d) "qt5": same as (a), including exception
(e) "kf5": same as (a), including exception

So we really have two groups:

(a,d,e): Remaining problem is ButtonUp on text, not CheckBox. This may be fixed, seems to be done in vcl code of SvTreeListBox

(b,c): Switches on MouseDown, but is not handled by SvTreeListBox, so probably native widgets. I would guess to fix that, creation of that widgets would need/does set other flags as wanted (?) I have no idea where this happens or is done, or how/where to fix that though

So I will try to expand my suggested fix to (a,d,e) and get it work with ButtonUp on text...
Comment 13 Armin Le Grand (allotropia) 2021-07-29 17:50:38 UTC
What a wild hunt using SAL_LOG, +WARN and stuff - also tried SAL_NO_MOUSEGRABS but did not work reliably.
In the end found in SvTreeListBox::MouseButtonUp a fix tdf116675 that does the trick, but with wrong remembered SvTreeListEntry. Corrected that and it seems to work well - no longer switching other lines on MouseButtonUp.
Preparing commit...
Comment 14 Commit Notification 2021-07-31 16:37:21 UTC
Armin Le Grand (Allotropia) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ca7dab5d96e73b7b4b045e2460e0b2ee150757db

tdf#143114 Avoid StartDrag on TreeListBox when CaptureOnButton

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Commit Notification 2021-08-16 11:21:34 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/183f66f79b40b6159a0edcf1b49c3aa612c90525

Revert "tdf#143114 Avoid StartDrag on TreeListBox when CaptureOnButton"

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 16 Julien Nabet 2021-08-16 11:25:29 UTC
Let's remove target since I reverted the patch because of regression (see tdf#143749 and its dup)
Comment 17 Armin Le Grand (allotropia) 2021-08-17 07:52:15 UTC
@Julien, @Buojaga: Thanks for identifying and taking action on tdf#143749. I will take a look ASAP on why there gets a crash triggered now - which unfortunately did not happen @commit-time :-(
Comment 18 Armin Le Grand (allotropia) 2021-08-17 08:48:42 UTC
Could reproduce on kf5 (use SAL_USE_VCLPLUGIN=kf5) in draw/impress. Reason is that pImpl == nullptr in SvTreeListBox::MouseButtonDown while pImpl is accessed unprotected (no check).
pImpl is nearly never checked for nullptr except e.g. SvTreeListBox::Invalidate. Gets set using SvTreeListBox::InitTreeView() and that is called from constructor (SvTreeListBox::SvTreeListBox), so it should be always set...

Looking at caller ImplHandleMouseEvent shows that pChild and it's VclReferenceBase has mbDisposed==true, that explains stuff. Thus I suppose those calls to pChild in MouseNotifyEvent::MOUSEBUTTONDOWN in ImplHandleMouseEvent should not be done at all (?)...
Comment 19 Armin Le Grand (allotropia) 2021-08-17 09:03:02 UTC
Checked SvTreeListBox::MouseButtonDown, and indeed win gets disposed in call
    pImpl->MouseButtonDown( rMEvt );
so I guess Julien was already on a hot trace in https://bugs.documentfoundation.org/show_bug.cgi?id=143749#c4

2nd possibility would be to ask for isDisposed() after pImpl->MouseButtonDown call, should also work. Looks as if pImpl usages *should* be all checked when as it seems any call can dispose it, that && calls in the stack above. That this is not the case shows that this stuff always was fixed on try-and-error (aka no 'concept' to secure it). So I see no other choice as to do the same for now (?)
Comment 20 Armin Le Grand (allotropia) 2021-08-17 09:16:29 UTC
Since SvTreeListBox::MouseButtonUp also calls parent at end of function (pImpl->MouseButtonUp) I tend to do it equal in SvTreeListBox::MouseButtonDown...
Comment 21 Armin Le Grand (allotropia) 2021-08-18 07:44:53 UTC
Fix on https://gerrit.libreoffice.org/c/core/+/120593, waiting for comments.
Comment 22 Commit Notification 2021-08-18 16:49:53 UTC
Armin Le Grand (Allotropia) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/eaac26c886a793dfb6c619a32a9fdeef98af3f79

tdf#143114 Avoid StartDrag on TreeListBox when CaptureOnButton

It will be available in 7.3.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 23 Jan-Marek Glogowski 2021-09-07 21:35:53 UTC
@alg: Is there something still missing from your POV or can we close it?
Comment 24 Michael Weghorn 2021-10-12 10:12:12 UTC
(In reply to Jan-Marek Glogowski from comment #3)
> (3) should be fixed by commit 63f92f185b78db5a575da58efac3f94a8cb5a5f6 ("Qt5
> fix Qt::Popup window handling")

Note that that commit (and others in the series) has been reverted in 7-2 branch (but not on master so far) to fix the Wayland-specific tdf#144585, s. tdf#144585 and https://gerrit.libreoffice.org/c/core/+/122356 for more details.

Interestingly, even with the commit being reverted (with current 7-2 status from bibisect repo), popups only remain open on X11, not in a Plasma Wayland session (at least in my tests on current Debian testing).
I used the toolbar item to insert a table in Writer for testing.
Comment 25 Jan-Marek Glogowski 2022-04-08 08:43:43 UTC
This needs re-evaluation, as more stuff has been fixed in general in the Qt VCL plugin. The reported bugs were supposed to be fixed, but the implementation caused many more problems then expected. And nobody is currently assigned, so switch to  NEW for the moment.
Comment 26 QA Administrators 2024-04-08 03:13:53 UTC Comment hidden (obsolete)
Comment 27 Michael Weghorn 2024-07-10 06:26:21 UTC
(In reply to Jan-Marek Glogowski from comment #25)
> This needs re-evaluation, as more stuff has been fixed in general in the Qt
> VCL plugin. The reported bugs were supposed to be fixed, but the
> implementation caused many more problems then expected. And nobody is
> currently assigned, so switch to  NEW for the moment.

Since this ticket mentions multiples things at the same time, let's follow the suggestion from comment 4 to have a clear scope:

(In reply to Jan-Marek Glogowski from comment #4)
> My suggestion is to test (1) and (3), then close this and open a new report
> for (2). And we need something that defines the expected behavior, before we
> can even start implementing a "fix".

-> Closing this ticket as fixed.
@Armin: Please open a separate ticket for (2) if this is still an issue.