Bug Hunting Session
Bug 117063 - Double-click does not expand/collaps the options in the navigator
Summary: Double-click does not expand/collaps the options in the navigator
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: UI (show other bugs)
Version:
(earliest affected)
6.1.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.1.0 target:6.2.0 target:6.1.0.2
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Navigator
  Show dependency treegraph
 
Reported: 2018-04-17 11:52 UTC by Xisco Faulí
Modified: 2018-11-26 02:27 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Xisco Faulí 2018-04-17 11:52:26 UTC
Steps to reproduce:
1. Open Writer
2. Insert an image or a drawing object
3. Open the navigator (F5)
4. Double click on images or drawing object

Observed behaviour: ListTree is not expanded.

Reproduced in

Version: 6.1.0.0.alpha0+
Build ID: b36ea44dcbdb862b0ac6e6cdaf27225b43dc0c7e
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group
Comment 1 Xisco Faulí 2018-04-17 11:53:17 UTC
Regression introduced by:

author	Jim Raykowski <raykowj@gmail.com>	2018-01-01 14:52:41 -0900
committer	Thorsten Behrens <Thorsten.Behrens@CIB.de>	2018-01-11 11:30:08 +0100
commit	582b2ed5ba25657afc2c2d45860899325b3b2450 (patch)
tree	60ec98ab10f80fb2da3a08ab8beb3d73e3c344b7
parent	f66fbd947f70f6be6b22ab372facaeb9e2fb63ae (diff)
tdf#36308 make double click not expand/collapse node in Navigator tree

Bisected with: bibisect-linux64-6.1

Adding Cc: to Jim Raykowski
Comment 2 Jim Raykowski 2018-04-18 02:53:57 UTC
Hi Xisco,

The intent of tdf#36308 was to do just this. It requires that the tree node icon to be clicked to expand/collapse. For Headings navigation this has been considered an improvement. I agree that this is a regression of behavior for the other navigation elements. 

Here is a patch that provides for double click to expand/contract for navigation elements other than content type Headings:

https://gerrit.libreoffice.org/#/c/53074/


This has required a change to a recent patch 

commit 1b9af08481b8f7f4bd15a30508606dff56b8e74f
Author: Jan Holesovsky <kendy@collabora.com>, Tue Mar 13 12:39:11 2018 +0100 (5 weeks ago)
Committer: Jan Holesovsky <kendy@collabora.com>, Tue Mar 13 16:28:40 2018 +0100 (5 weeks ago)
Follows: libreoffice-6-0-branch-point
Branches: <Expand>

tdf#116334: Actually when there is no handler, we have to return 'true'.

Also use the return value of the link's Call() [as the SvTreeListBox::ExpandingHdl()
is doing], the appropriate callbacks seem to return the expected 'false' in
the cases I've reviewed...

Change-Id: I0cdd63e8ec4c794839070b914150e0b32f743359
Reviewed-on: https://gerrit.libreoffice.org/51211
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Jan Holesovsky <kendy@collabora.com>

to what the patched code was two patches prior 

treelistbox.cxx:428 
bool SvTreeListBox::DoubleClickHdl()
{
    aDoubleClickHdl.Call( this );
    return true;
}

This is because if SvTreeListBox::DoubleClickHdl() does not return true

svimplbox.cxx:2024 
if( pView->DoubleClickHdl() )

will not be entered and 

svimplebox.cxx:2041
if( bSubLstOpDblClick )

can not be checked to decide if the element is one where it's sublist should be expanded/collapsed on doubleclick.

We better check with Jan on this.
Comment 3 Xisco Faulí 2018-04-18 08:42:48 UTC
Hi Jim,
Thanks for the detailed explanation.
I reported it because it looked like a bug to me, but as you're saying, the logic behind 582b2ed5ba25657afc2c2d45860899325b3b2450 is to disable the expand/collapse, thus, this bug will be a NOTABUG.

Whether we should enable it for all elements other than headings or not, I would prefer to hear the UX team's opinion first...
Comment 4 Heiko Tietze 2018-04-19 15:16:27 UTC
Not sure if we should implement different interactions (requested here is to keep the change for headings where the node state is retained on double-click but to open the parental nodes in all other cases, e.g. Images).

The user should learn whether double-click expands or not. Any dependency on the type of content, even when it's perfectly clear in this case, adds complexity to the UI and not all beginners can follow the reasoning behind different workflows.

IMHO, WF (open for discussion)
Comment 5 Jim Raykowski 2018-04-21 01:59:23 UTC
When I orignally read this report I was of the opinion WF becuase we fixed it already. After testing the patch I was more in favor of using it's behavior. As far as I know double click is used to expand/collapse all other lo tree lists but the one in the Navigator.
Comment 6 Xisco Faulí 2018-04-24 18:29:14 UTC
@Heiko, could it be discussed in tomorrow's UX meeting ?
Comment 7 Cor Nouws 2018-04-25 18:37:03 UTC
testing in Version: 6.1.0.0.alpha0+
Build ID: 66c02d16dd078613e754dcc775f366413fad13f0
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk2; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-04-20_09:08:39
Locale: nl-NL (nl_NL.UTF-8); Calc: group


I see no different behavior for Headings, so remove "( other than the headings )" from the summary
Comment 8 Heiko Tietze 2018-04-26 13:31:31 UTC
We discussed the topic in the design meeting and came to the conclusion to follow Jim's comment 5. Root nodes (including "Headings" but not the actual headings below) open on double click, child nodes don't.
Comment 9 Heiko Tietze 2018-04-29 06:47:06 UTC
Thanks, Jim!
Comment 10 Commit Notification 2018-04-29 06:48:07 UTC
Jim Raykowski committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b649ce123dea372359ec571135a68eb3de844e5b

tdf#117063 Modify tree list double click behavior in the navigator

It will be available in 6.1.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 11 Xisco Faulí 2018-05-02 10:45:10 UTC
Verified in

Version: 6.1.0.0.alpha1+
Build ID: 43af818557904450b13839350c65ad865b9ee9d2
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group
Comment 12 Jan Holesovsky 2018-05-03 17:22:43 UTC
Jim: Thank you for working on this - it's pity you didn't CC me in this bug when you've done the analysis in comment 2...

Returning true unconditionally in SvTreeListBox::DoubleClickHdl() I'm afraid re-introduces bug 115950.

The root of the problem here is that many of the double-click handlers return a wrong value.  Now I have a list of places where that happens thanks to a compiler plugin, so I'll go through them, and fix accordingly - then I'm likely to change the code in SvTreeListBox::DoubleClickHdl() back to what it was before your b649ce123dea372359ec571135a68eb3de844e5b .

Seeing the amount of regressions, I agree that for the moment it is better to return true unconditionally for the moment :-) - thanks for that!
Comment 13 Jim Raykowski 2018-05-04 01:13:34 UTC
(In reply to Jan Holesovsky from comment #12)
> Jim: Thank you for working on this - it's pity you didn't CC me in this bug
> when you've done the analysis in comment 2...

Jan, Sorry about that. I thought one of the bugzilla gurus would get a message to you. They do an outstanding job. Many kudos to them! I'm still sort of a beginner here and don't know how to CC.
Comment 14 Jan Holesovsky 2018-05-04 08:37:02 UTC
Jim - don't worry at all!  You are doing great, and it is awesome to have you contributing code :-) - thank you for that!
Comment 15 Commit Notification 2018-07-12 10:44:35 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c9d1655d455ad783694e6d7f0d2e6cf3d0d0acae

Resolves: tdf#115950 proper double click return and bail out, tdf#117063

It will be available in 6.2.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 16 Commit Notification 2018-07-12 11:25:57 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=08ac6e6f991cffcb1fed7bd1bb4ec67e94817507

SvTreeListBox::DoubleClickHdl: Navigator return more to be done, tdf#117063

It will be available in 6.2.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 17 Commit Notification 2018-07-12 11:45:54 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-6-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=172817aa670ff923780d0159c4416871179fa94c&h=libreoffice-6-1

Resolves: tdf#115950 proper double click return and bail out, tdf#117063

It will be available in 6.1.0.2.

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

Affected users are encouraged to test the fix and report feedback.
Comment 18 Commit Notification 2018-07-12 16:32:40 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-6-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c73b9d69abf1b26dc2c6f9d6700b2a5f2c9e6f36&h=libreoffice-6-1

SvTreeListBox::DoubleClickHdl: Navigator return more to be done, tdf#117063

It will be available in 6.1.0.2.

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

Affected users are encouraged to test the fix and report feedback.