Elide cut off text in sidebar header, remove restricted max width
AbandonedPublic

Authored by rkflx on Jul 14 2017, 7:18 AM.

Details

Reviewers
sander
pino
aacid
Group Reviewers
Okular
Summary

In some languages the sidebar container header is wider than the
minimum container width and therefore cut off. By using a squeezable
label and setting a proper default width, the header is displayed fully
when Okular is started for the first time, while still being able to
reduce the size via the splitter.

By removing the explicitly set minimum width, the QVBoxLayout is
now allowed to actually do its job of automatically calculating the
proper minimum width based on its contained widgets.

In times of ultrawide 21:9 displays, setting the maximum width to 600px
seems arbitrary and overly restrictive, so it is removed.

Both width restrictions were originally chosen in 078b81b7 with no
explanation given for the values selected, which later were changed to
different values multiple times. Today, they simply do not seem
necessary anymore.

Note: The first-start size for some languages is now wider than before,
still acceptable though (see screenshots of Greek, where the translated
string of "Thumbnails" is one of the longest among all languages).
Most languages are not affected at all and users could easily move the
splitter further to the left.

Note: KF5_REQUIRED_VERSION is not bumped for now to require the fix to
KSqueezedTextLabel (see depending review), as this minor visual tweak
should not prevent building of Okular on older distros. For those, at
least cutoff will be reduced to 9px, distros with updated KF5 would get
the full cutoff reduction to 0px.

BUG: 176780

Depends on D7164

Test Plan

Use default fonts, change application language to Greek. Remove
Okular's config file before restarting. Notice how sidebar header is
not cut of anymore. Move splitter left (text in header should elide) and
right (does not stop at 600px). Window resizing works as expected.

Before (cut off)After (minimum elided)After (first start)

Diff Detail

Repository
R223 Okular
Branch
arcpatch-D6696 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
rkflx created this revision.Jul 14 2017, 7:18 AM
Restricted Application added a project: Okular. · View Herald TranscriptJul 14 2017, 7:18 AM
aacid requested changes to this revision.Jul 24 2017, 2:40 PM
aacid added a subscriber: aacid.

I would very much prefer if you would use QGuiApplication::devicePixelRatio instead of removing the lines.

This revision now requires changes to proceed.Jul 24 2017, 2:40 PM
rkflx added a comment.Jul 24 2017, 4:51 PM

Thanks for looking at my patch, but I'm not sure I'm understanding you completely here (you didn't state a reason for your preference). How would this fix the header text being cut off and the (seemingly) arbitrary max width restriction? Hardcoding any fixed widths (even with HiDPI scaling in mind) seems wrong.

Removing the lines, the sidebar adapts its width to the header text. In practice, apart from eliminating visual glitches, only little will change, e.g. for English, both default and minimum sidebar widths are nearly identical (if not a little smaller).

aacid added a comment.Jul 25 2017, 9:24 AM

My preference is, you actually fix the code instead of removing it.

rkflx planned changes to this revision.Jul 30 2017, 6:43 PM
sander added a subscriber: sander.Jul 31 2017, 11:27 AM

Albert, I don't see how fixing the code would look like, either. Even if the max width would be given in some dpi-independent way, there would still be a size limit and translations of "Thumbnails" would be cut off. Are you proposing to use an dpi-independent size unit AND increasing the max size?

aacid added a comment.Jul 31 2017, 8:13 PM

Albert, I don't see how fixing the code would look like, either. Even if the max width would be given in some dpi-independent way, there would still be a size limit and translations of "Thumbnails" would be cut off. Are you proposing to use an dpi-independent size unit AND increasing the max size?

Yes and no, what i am saying is
d->sideContainer->setMaximumWidth( 600 * QGuiApplication::devicePixelRatio );
since QGuiApplication::devicePixelRatio will be something like 2, yes the max size is increased, but not "really increased", because what happens is that everything is twice as big

sander added a comment.Aug 1 2017, 7:59 AM

Ah, I get it. But as rkflx I don't see the point of the size limit. The sideBar contains five known fixed-but-translated words. Either the size limit is too small for all possible translations. Then some of these translations look ugly. Or it is large enough, but then the size limit is pointless and could be removed.

rkflx added a comment.Aug 1 2017, 8:12 AM

I guess this discussion is more about behaviour than about
implementation. Should we artificially restrict min and max widths? IMHO
no, and the HIG doesn't talk about it either.

As for the potential change in min width (see screenshots above for most
extreme case): We could use elided text and set a non-eliding default
width. I'm working on this compromise, as indicated by the status
"Changes Planned". However, this will probably need a bugfix in KF5 first.

aacid added a comment.Aug 2 2017, 9:13 PM
In D6696#130719, @rkflx wrote:

Should we artificially restrict min and max widths? IMHO no,

You have an opinion, I have another.

the HIG doesn't talk about it either.

the HIG is not exhaustive at all, lots of programs don't do things that the HIG doesn't talk about.

rkflx added a comment.Aug 6 2017, 12:36 PM
setMaximumWidth( 600 * QGuiApplication::devicePixelRatio )

Suprisingly, this is not needed at all. The scaling is already working in HiDPI mode, still restricted to 600px*scalefactor though. I'll modify my argument to concern ultrawide screens so it still stands, along with the cutting-off problem.

Should we artificially restrict min and max widths?

I'm genuinely interested in your reasoning for keeping min and max width restrictions. Maybe I have missed an important aspect in my arguments against them? I'd be glad if you can point it out, so I could learn something and improve my patch.

rkflx updated this revision to Diff 17777.Aug 6 2017, 12:38 PM
rkflx edited edge metadata.

use elided text and set a non-eliding default

Updated patch accordingly.

rkflx retitled this revision from Fix cut off text in sidebar header, remove restricted max width to Elide cut off text in sidebar header, remove restricted max width.Aug 6 2017, 12:41 PM
rkflx edited the summary of this revision. (Show Details)
rkflx edited the test plan for this revision. (Show Details)
aacid added a comment.Aug 7 2017, 8:23 PM

Can you explain why you would let people resize the sidebar to stupidly unusable sizes?

rkflx added a comment.Aug 14 2017, 5:14 PM

stupidly unusable

I find it really hard to translate this kind of comment into actionable feedback I can improve upon, because this neither details an actual problem, nor does it suggest an alternative size.

Instead, let me expand on the properties of my proposed solution, so we can determine any flaws:

For the minimum size:

  • determined by the widgets inside the sidebar container
  • large enough to still allow for usage of all functionality
  • small enough to not get in the way when space is tight
  • elision of header text if needed
  • in practice ~90px

For the maximum size:

  • not restricted to a hardcoded size
  • nevertheless, the actual document itself is never hidden completely
  • accommodates long TOC entries, reviews and bookmarks, the user gets to choose the distribution of available space to sidebar and document for herself
  • for horizontally wide documents, e.g. construction plans, moving the dark rectangle in the thumbnail preview with the mouse to pan over the document is easier for larger widths
  • if a user does not like the thumbnail being large, she can always reduce the size of the sidebar
  • removal of restriction is precondition if we were to implement a zoomable thumbnail grid (which seems standard in other viewers)
  • today's ultrawide 21:9 displays have widths of up to 3.440 pixels, Okular's sidebar should be flexible within these bounds and not restricted to 600px
  • no reasonable argument for restricting in commit history, HIG, bugzilla and this review (besides a single "opinion")
  • users are asking for it (see bug) and may have workflows and documents we currently do not cater for
  • other free and proprietary document viewers do not restrict the maximum sidebar size either (I tested GNOME's document viewer, Adobe Reader and Foxit Reader)
  • other KDE apps are unrestricted too, e.g. Dolphin, Gwenview and KMail

If there are any other comments regarding my patch, please share.

aacid resigned from this revision.Aug 15 2017, 8:25 AM

Honestly i don't see a reason why http://i.imgur.com/K2ap7CD.png is a layout we should let the user set, but as said, it's just my "opinion".

Find someone else to commit this for you :)

Thanks for the screenshot, now I know you are talking about the max width of the thumbnail sidebar for vertical documents. I guess you are fine with removing the min width.

As said above, I think users are perfectly capable to decide for themselves which width they like. Indeed, the grid approach would be better, but is out of scope for this patch. We should not focus only on the thumbnails here, but also look at e.g. the TOC.

Lastly, it is the nature of interfaces with flexible layout to not perform optimally in every situation, i.e. display resolution, aspect ratio and type of document. A static max width does not work, it should (if at all) depend on the context.

I will now try to find other reviewers. If they find the single edge case pictured above to be more important than the general case (e.g. non-thumbnail sidebar, different document aspect ratios, users acting rationally) we could try to limit the sidebar width to half the window size.

rkflx added a subscriber: pino.

@pino: arc cover and git blame printed your name. If you find the time, I'd be happy if you could take a look at my patch.

@sander: Do you happen to know other reviewers who have an interest in Okular?

sander edited edge metadata.Aug 15 2017, 10:21 AM

I agree with Henrik again. Yes, the very wide thumbnails in Albert's screenshot are strange, but I don't see why we should actively prohibit them. Besides, even with a vanilla Okular I am able to make the thumbnails about twice as wide as the actual pages (screenshot on request). So Henrik's patch does not make things (much) worse in that regard.

rkflx added a comment.Aug 19 2017, 1:27 PM

Thanks Oliver. I'll respect the rules and wait for formal acceptance before committing, though.

rkflx added a comment.Sep 10 2017, 7:17 PM

@pino Friendly ping :)
Please let me know if you have any questions or someone else should review this.

I just tried the patch. On my German setting, the labels are usually also cut off to the left and to the right. Strangely, the patch doesn't really change that.

rkflx added a comment.Sep 11 2017, 9:35 AM

I just tried the patch. On my German setting, the labels are usually also cut off to the left and to the right. Strangely, the patch doesn't really change that.

I think you mixed something up :)

Your screenshot shows a bug with the sidebar entries (multiple), which is something I propose to fix in D6695 (please comment there with scaling, font, font size and subpixel hinting settings if D6695 still does not work for you). However, here we are looking at the (single) bold sidebar header (right below "Bearbeiten").

Ah, now I see it. Yes, the patch seems to work: the sidebar header is now elided rather than simply cut off, and I can make the sidebar as large or as small as I desire.

Albert, I consider committing this, unless you really object. What do you say?

(D6695 works for my case.)

ngraham added a subscriber: ngraham.EditedOct 19 2017, 3:18 AM

*raises hand* I care about Okular!

I actually have to agree with Albert here, and I have what I believe to be a good reason for not allowing the thumbnail bar to be wider than the main view.

In the screenshot Albert provided, it wasn't clear to me that the thumbnail bar was hugely wide. I actually thought that he had somehow moved the thumbnail bar to the right side and what was in the middle was the main content view.

Any novice user who accidentally resizes the thumbnail bar in such a manner would be similarly confused by the different interaction patterns, and it would probably never occur to them to try resizing it to resolve the issue. I can imagine my mother doing this--she actually does this sort of thing all the time, and it always results in a phone call to me.

I do like the idea of eliding the header text at small widths, though. That seems like a good change.

rkflx planned changes to this revision.Oct 20 2017, 9:30 PM

Thanks Nate, you make a rational point (even if I do not agree fully). As promised above:

I will now try to find other reviewers. If they find the single edge case pictured above to be more important than the general case (e.g. non-thumbnail sidebar, different document aspect ratios, users acting rationally) we could try to limit the sidebar width to half the window size.

…which I will now add to my queue of things to try to implement (won't be trivial, as the window size can change at runtime).

FWIW, Apple's Preview does something similar and limits the size of the sidebar to about 40% of the window's width. They elide the text, too.

FWIW, I don't think eliding the text is controversial at all; could we maybe break that out into a new patch so we can get that merged quickly?

rkflx abandoned this revision.Aug 24 2018, 10:47 PM
Restricted Application added a subscriber: okular-devel. · View Herald TranscriptAug 24 2018, 10:47 PM