TOC: Add collapse/expand options
ClosedPublic

Authored by Lekensteyn on Aug 17 2018, 4:40 PM.

Details

Summary

Large specifications with many (nested) sections are painful to navigate
through when the TOC is expanded by default. Introduce four new options,
"Expand/Collapse whole section" is based on Kate's document view while
"Expand/Collapse all" was added to handle the top-level sections.

As for other viewers, PDF.js uses shift-click to handle the former while
using double-click on a the TOC icon to handle the latter. That is not
very obvious, so extending the context menu seems the next best option.

BUG: 216870

Test Plan

Open a document such as ACPI 6.2 (Eratta A) from https://uefi.org/specifications
Try Expand/Collapse all. Select a child and try Expand/Collapse recursively whole section.
(revision with different text/no icons:)


(from revision 1:)

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Lekensteyn created this revision.Aug 17 2018, 4:40 PM
Restricted Application added a subscriber: okular-devel. · View Herald TranscriptAug 17 2018, 4:40 PM
Lekensteyn requested review of this revision.Aug 17 2018, 4:40 PM
ngraham requested changes to this revision.Aug 20 2018, 12:50 PM
ngraham added a subscriber: ngraham.

Thanks for the patch! +1 conceptually, but I'd like to see a few UI changes here.

"Recursively" is a programmer jargon word that most people don't understand. How about these instead:

Expand this section
Collapse this section
Expand everything
Collapse everything

Also, could we put a separator between the first two items and the second two items?

This revision now requires changes to proceed.Aug 20 2018, 12:50 PM

Hi Nathaniel, thank you for your feedback!

"Recursively" is a programmer jargon word that most people don't understand.

It was copied from Kate and sounded natural to me, but there is likely some bias there ;)

How about these instead:

Expand this section
Collapse this section

Couldn't this be confused for the default action (collapsing a single item)? "Expand whole section" seems more accurate, but it still does not sound perfect though. What about "Expand subtrees"?

Expand everything
Collapse everything

Also, could we put a separator between the first two items and the second two items?

As the actions are very closely related, I think that less separators would look better. I could still add it if you prefer that?

It was copied from Kate and sounded natural to me, but there is likely some bias there ;)

Aha! Then it would be nice to improve this for Kate too, once we settle on a final wording here. :)

Couldn't this be confused for the default action (collapsing a single item)? "Expand whole section" seems more accurate, but it still does not sound perfect though. What about "Expand subtrees"?

"Expand subtrees" is probably still too technical. I like "Expand/collapse whole section" though.

As the actions are very closely related, I think that less separators would look better. I could still add it if you prefer that?

I think the reason why my brain wants some separation there is because right now the four new items look very similar to one another in the menu, which impairs my ability to quickly scan. If you used different icons for the different actions, that would probably help.

Thanks for the patch.

  • I like 'Expand whole section', too.
  • I don't think additional separators are necessary.
  • Four times the same icon looks strange to me, but I don't know what the alternatives are.

None of this is blocking to me. @aacid , do you object to me merging this?

It was copied from Kate and sounded natural to me, but there is likely some bias there ;)

Aha! Then it would be nice to improve this for Kate too, once we settle on a final wording here. :)

kate is a programming tool (the non programming editor is kwrite) so having programming lingo in there is fine.

Thanks for the patch.

  • I like 'Expand whole section', too.
  • I don't think additional separators are necessary.
  • Four times the same icon looks strange to me, but I don't know what the alternatives are.

Don't have any icon?

None of this is blocking to me. @aacid , do you object to me merging this?

The braces are all in the wrong place, and @ngraham didn't seem to happy with the wording as far as i understand.

part.cpp
2996

Would "Collapse children" instead of just "Collapse" make more sense? For some reason my brain thought this would close up until the root when i read "Collapse recursively"

Lekensteyn edited the summary of this revision. (Show Details)
Lekensteyn edited the test plan for this revision. (Show Details)

I changed the text and removed the icons as I was not able to find a suitable replacement in /usr/share/icons (no results for expand|collapse, "tree" did not have good candidates.)

It looks a bit naked without icons though.

The braces are all in the wrong place

Which braces?

part.cpp
2996

"Collapse all" would close every section, "Collapse recursively" intended to work downwards instead of to the top (what would the use case be for the latter?)

Also, considering the non-programmers, "Collapse children" might be a confusing message.

ngraham accepted this revision.Aug 23 2018, 1:39 PM

I'm good with this now. I wonder if "Expand/collapse everything" or "Expand/collapse all sections" might be clearer than "Expand/collapse all", but that's just a tiny minor probably subjective thing and I don't know if it's worth

This revision is now accepted and ready to land.Aug 23 2018, 1:39 PM
aacid added a comment.Aug 23 2018, 5:32 PM

The braces are all in the wrong place

Which braces?

if (!worklist[0].isValid()) {
       return;
   }

for (int i = 0; i < m_model->rowCount(index); i++) {

The style for the files you've touched is [mostly] braces on next line, not same line

The style for the files you've touched is [mostly] braces on next line, not same line

I see. The file also seems to prefer patterns like:

if( condition )
{
    functionCall( arg );
}

although some also put an additional space in if ( condition ). What kind of style should I apply here? I do not like this style (where does this come from?), but inconsistency is even worse.

This revision was automatically updated to reflect the committed changes.