Correct margin for left sidebar
ClosedPublic

Authored by kapillamba4 on Dec 3 2017, 4:35 PM.

Details

Summary

Sidebar content too close to the left boundary of the window.

This patch adds left margin to some of the sidebar contents.

BUG: 381535

Test Plan

Before:


After:

Diff Detail

Repository
R260 Gwenview
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham accepted this revision.Dec 3 2017, 4:38 PM

You fixed this, yay! I was tearing my hair out trying to figure out what the issue was. This works and looks good to me, but I'll hold off on landing it until others have had a chance to comment on whether or not this is the right implementation.

This revision is now accepted and ready to land.Dec 3 2017, 4:38 PM
rkflx requested changes to this revision.Dec 3 2017, 4:53 PM

Great fix, was on my list of things to improve, too :)

I'll give this a thorough test tomorrow (only looked at the screenshot so far, need to build and look at the code in detail still). But first two questions:

  • Does this work with HiDPI displays? (To test, use QT_SCALE_FACTOR=2 gwenview)
  • Would it be possible to not hardcode 3 all over the place? (Look around a bit, there are already other constants defined. Do it like those.)
This revision now requires changes to proceed.Dec 3 2017, 4:53 PM

The patch works fine for me in HiDPI mode FWIW.

Seems to work with HiDPI for me as well:

kapillamba4 added a comment.EditedDec 3 2017, 5:28 PM

I only see margin "3" at 4 places, 3 of which will be added by this patch and
most of the "margin" values seem to be zero.

rkflx added a comment.Dec 3 2017, 5:31 PM

I only see margin "3" at 4 places, 3 of which will be added by this patch and
most of the "margin" values seem to be zero.

Why I mean is that you should do something like this.

kapillamba4 updated this revision to Diff 23390.Dec 3 2017, 6:04 PM
kapillamba4 updated this revision to Diff 23391.Dec 3 2017, 6:12 PM
kapillamba4 retitled this revision from Correct margin for Left Sidebar and fix icon placement in Sidebar buttons to Correct margin for Left Sidebar and fix icon placement in Sidebar buttons in gwenview..Dec 3 2017, 6:20 PM
kapillamba4 retitled this revision from Correct margin for Left Sidebar and fix icon placement in Sidebar buttons in gwenview. to Correct margin for Left Sidebar and fix icon placement in Sidebar buttons for gwenview..
rkflx added a comment.Dec 4 2017, 11:51 PM

Thanks for the updates, much cleaner code :) In general I feel this could be done in an even simpler way, but haven't got the time really to figure this out right now. Maybe I'll send a follow-up patch.

Regarding the commit message: BUG: ... will automatically close the corresponding Bugzilla entry, however this only works when it is on a separate line (currently the text representation of the "Details" from above will become Summary: BUG: ... in the commit message, which won't work for this). Please add a short sentence in an extra line before BUG: ..., which would be good anyway e.g. to describe what the problem was before / why it was necessary to correct the margin.

Please see inline comments for some additional niggles:

app/mainwindow.cpp
537

I don't think we need this for the Folders sidebar. The text already has enough margin, and if someone chooses to have no window border, we should not try to introduce an extra "border" inside the app. The right and bottom borders look ugly now, too.

548

Same thing for the Information sidebar. Here, only the part above the text input widget should be indented more, and also only on the left (see next comment).

556

In general I do not like how this adds more space everywhere at the top, right and bottom. We only want the wider margin on the left, don't we?

Maybe you could try something like setContentsMargins instead.

app/sidebar.cpp
158

This caught my eye, because setting a non-quadratic icon size seems like a hack. And indeed, this leads to much more spacing now between icon and text. Could you try to find a way so the spacing looks basically the same as in the toolbar button at the top of the following screenshot?:

If this turns out to be too difficult, I'm willing to accept the current state. But at least we should try to be consistent here before giving up…

ngraham added inline comments.Dec 5 2017, 3:04 AM
app/sidebar.cpp
158

Nice catch, @rkflx. It would indeed be great if we can move the icons over a bit to match the typical icon placement within a toolbutton.

kapillamba4 added a comment.EditedDec 5 2017, 12:05 PM

@ngraham , @rkflx does this look good?

3 padding between icon and text, 3 padding between icon and button boundary.
I' ll create a patch soon with other changes as well.

kapillamba4 edited the summary of this revision. (Show Details)Dec 5 2017, 12:13 PM
rkflx added a comment.EditedDec 5 2017, 12:16 PM

does this look good?

The space between icon and text now looks much narrower than without the patch. I think it was just fine in an unpatched Gwenview, it is only the left margin which needs tweaking. Note that in my screenshot I set the toolbar icon size to "small" to match the sidebar buttons. You can use pixeltool-qt5 or gammaray to make measuring easier.


As for the summary, please don't just repeat the title ;) Write what was wrong (i.e. section header text as well as buttons too close to edge of window when setting window borders to "none") and what fixes this.

kapillamba4 updated this revision to Diff 23573.Dec 6 2017, 3:59 PM
kapillamba4 edited the summary of this revision. (Show Details)
kapillamba4 marked 5 inline comments as done.Dec 6 2017, 4:05 PM
In D9145#176147, @rkflx wrote:

does this look good?

The space between icon and text now looks much narrower than without the patch. I think it was just fine in an unpatched Gwenview, it is only the left margin which needs tweaking. Note that in my screenshot I set the toolbar icon size to "small" to match the sidebar buttons. You can use pixeltool-qt5 or gammaray to make measuring easier.


As for the summary, please don't just repeat the title ;) Write what was wrong (i.e. section header text as well as buttons too close to edge of window when setting window borders to "none") and what fixes this.

gammaray is awesome :)

HiDPI:

I still don't know why normal conventional methods like setting stylesheet or setContentsMargin are not working for QToolButton

Lovely, much better now!

kapillamba4 added a comment.EditedDec 6 2017, 4:23 PM

Lovely, much better now!

yup, 👍

rkflx added a comment.Dec 6 2017, 6:39 PM

Thanks, I'll look at your updates in a bit.

Meanwhile, please let us know which email address should be used when landing this patch on your behalf.

In D9145#176861, @rkflx wrote:

Thanks, I'll look at your updates in a bit.

Meanwhile, please let us know which email address should be used when landing this patch on your behalf.

kapillamba4@gmail.com

rkflx added a comment.Dec 6 2017, 10:11 PM

Much better, but I think we need one more iteration regarding the spacing (did not yet look at the code in depth).

I still don't know why normal conventional methods like setting stylesheet or setContentsMargin are not working for QToolButton

Sorry, don't know that either. When looking around, you do not seem to be the first to notice this, though.

app/mainwindow.cpp
537

That's not done. What I meant was that you should not touch the spacing here at all. If you look at Dolphin, the main folder view also extends all the way to the right window border.

In addition, this does not align well with the left border of the tab strip below (w/o patch it was aligned perfectly).

548

I don't think this can be marked as "Done", because the Description text input widget is still affected by your patch, while it shouldn't be.

It is only the text above this widget which should have extra margin added to the left.

app/sidebar.cpp
158

Spacing now looks almost like it was designed by a professional designer, good job!

However, with QT_SCALE_FACTOR=1.4 (i.e. fractional scaling), the rendering quality of the icons degrades massively (note that the toolbar is still fine):

(Patch on the left, 17.08.3 on the right).

rkflx added inline comments.Dec 6 2017, 10:21 PM
app/sidebar.cpp
158

See also D7581 and the API docs of drawPixmap. Maybe using the function call with ...F (floating point) parameters will help here?

kapillamba4 added a comment.EditedDec 6 2017, 10:43 PM

I don't think fractional scaling is supported by qt:
https://www.mail-archive.com/kde-devel@kde.org/msg09355.html
https://bugzilla.redhat.com/show_bug.cgi?id=1381828#c1
https://bugreports.qt.io/browse/QTBUG-55654

I also tried using floating point values for drawing pixmap and the result was much worse.

rkflx added a comment.Dec 6 2017, 10:52 PM

Skimming over those links, I get the impression those are about general scaling issues or autodetection. As you can see in my screenshot, fractional scaling works just fine. It is only your patch that makes it worse unfortunately.

kapillamba4 updated this revision to Diff 23599.Dec 7 2017, 7:53 AM

For QT_SCALE_FACTOR=1.4 :

This comment was removed by kapillamba4.
rkflx added a comment.EditedDec 7 2017, 5:32 PM

I'm so sorry I have to tell you this, but now it breaks in another place. I'm confident you'll get it right eventually :)

Also, I guess you are still working on the margins of the text input widgets as per my comments?


Edit: Can't edit the inline comment, of course I mean "do NOT remove"…

app/sidebar.cpp
89

That's a horrible hack which breaks gwenview -reverse (compare the text alignment w/ and w/o patch).

Also, I'm not sure how this interacts with screen readers. I think it is best if you do remove the text internal to the button, so screen readers, RTL writing systems and potentially other things continue to work.

As I said in D9145#176020, I would be okay with the version referenced there if this turns out to be too difficult.

kapillamba4 added a comment.EditedDec 7 2017, 7:18 PM
In D9145#177316, @rkflx wrote:

I'm so sorry I have to tell you this, but now it breaks in another place. I'm confident you'll get it right eventually :)

Also, I guess you are still working on the margins of the text input widgets as per my comments?


Edit: Can't edit the inline comment, of course I mean "do NOT remove"…

Now i am out of ideas :|
Only small hacks like these seem to work, I think the first revision: D9145#176020 is much safer to use. Even if we think of any other way to resolve this problem, it is going to break gwenview in some way which we might not find right now.
Also my university exams are going on right now, so i am not able to devote much time to this issue.

kapillamba4 added a comment.EditedDec 7 2017, 7:28 PM

I actually don't think text input widgets look bad with or without margin.

Still if you say i will remove the margin :)

rkflx added a comment.Dec 7 2017, 9:20 PM

I think the first revision: D9145#176020 is much safer to use. Even if we think of any other way to resolve this problem, it is going to break gwenview in some way which we might not find right now.

Ok, then we agree. At least we tried and learned something in the process…

Also my university exams are going on right now, so i am not able to devote much time to this issue.

No need to rush, the earliest release this patch could ship would be 17.12.1 on January 8.

I actually don't think text input widgets look bad with or without margin.

Still if you say i will remove the margin :)

It looks bad in the default config (i.e. with normal window borders enabled), and regardless of window border size the left border of the text widget should be as close to the window border as the left border of the tab strip, to replicate the alignment from the right side. So yes, I would prefer if you removed the left margin from both text widgets. I guess this might boil down to not adding any margin to the container at all, and only indenting the text above a bit.

In D9145#177362, @rkflx wrote:

Ok, then we agree. At least we tried and learned something in the process…

yup

In D9145#177362, @rkflx wrote:

So yes, I would prefer if you removed the left margin from both text widgets.

I can see only one text input widget (in Information tab)

Thanks for the updates, I'll do the final review later today or tomorrow.

So yes, I would prefer if you removed the left margin from both text widgets.

I can see only one text input widget (in Information tab)

My bad, I meant "widgets with white background and blue frame", i.e. what you'll find on the Information and Folders panels.

app/sidebar.cpp
162

Can you remove this? I think I found a way to fix the problem properly. Still needs some fiddling, but I'll have it figured out soon™ (sorry for the cliffhanger :)

kapillamba4 marked an inline comment as done.
kapillamba4 retitled this revision from Correct margin for Left Sidebar and fix icon placement in Sidebar buttons for gwenview. to Correct margin for Left Sidebar for gwenview..
kapillamba4 edited the summary of this revision. (Show Details)
kapillamba4 added a comment.EditedDec 10 2017, 2:24 PM
In D9145#178033, @rkflx wrote:

Can you remove this? I think I found a way to fix the problem properly. Still needs some fiddling, but I'll have it figured out soon™ (sorry for the cliffhanger :)

I think i have fixed this finally :)


(Works for QT_SCALE_FACTOR = 1.4 and gwenview --reverse)

Does this look good?
I'll make some more changes and update the revision.

kapillamba4 retitled this revision from Correct margin for Left Sidebar for gwenview. to Correct margin for Left Sidebar and fix icon placement in Sidebar buttons for gwenview..Dec 10 2017, 2:25 PM
rkflx added a comment.EditedDec 10 2017, 2:27 PM

My fix won't be in Gwenview, you don't need to do anything here ;)

However, I wonder whether you uploaded the complete patch or missed a git add, because for me the Rating label is still too far to the left, while your screenshot is correct. Could you have a look?

kapillamba4 retitled this revision from Correct margin for Left Sidebar and fix icon placement in Sidebar buttons for gwenview. to Correct margin for Left Sidebar for gwenview..
rkflx added a comment.Dec 10 2017, 2:55 PM

The margin is still on the wrong side in RTL mode, but at least it interferes not as badly with the text as in a previous iteration and the sidebar headers are broken in that regard anyway. I don't want to overburden this patch, so you won't have to improve that for now :)

app/sidebar.cpp
162

Phab shows a whitespace change here (added empty line), please remove.

app/sidebar.h
40

const QMargins& margins would be better, but actually I don't really like this hacky way, because we have 4 headers, but you are calling this function only for one. This may be obvious now, but in several years nobody will understand the code.

I think the best way would be to try to get rid of semanticinfosidebaritem.ui, i.e. manually create its items in code and essentially have in each sidebar a container (red rectangle) which is then indented at a central place:

This comment was removed by kapillamba4.
kapillamba4 added a comment.EditedDec 10 2017, 6:16 PM
In D9145#178097, @rkflx wrote:

I think the best way would be to try to get rid of semanticinfosidebaritem.ui, i.e. manually create its items in code and essentially have in each sidebar a container (red rectangle) which is then indented at a central place:

Can we just set left-margin for title to 3 in constructor of sidebargroup, also set container margin to 3 for Image operation, file operation and meta information and set margin 3 inside ui file for semantic information (not whole of the container)

rkflx added a comment.Dec 10 2017, 6:30 PM

Can we just set left-margin for title to 3 in constructor of sidebargroup, also set container margin to 3 for Image operation, file operation and meta information and set margin 3 inside ui file for semantic information (not whole of the container)

Depending on what you refer to with "container", the title might have double margin now? Just try it, I guess.

If you want to keep the ui file, I would be okay with that. But instead of the margin setter call at least add a comment to that effect at a similar place in the code.

kapillamba4 added a comment.EditedDec 10 2017, 6:45 PM
In D9145#178152, @rkflx wrote:

Can we just set left-margin for title to 3 in constructor of sidebargroup, also set container margin to 3 for Image operation, file operation and meta information and set margin 3 inside ui file for semantic information (not whole of the container)

Depending on what you refer to with "container", the title might have double margin now? Just try it, I guess.

If you want to keep the ui file, I would be okay with that. But instead of the margin setter call at least add a comment to that effect at a similar place in the code.

what i mean by container is stuff below title like text input widgets or toolbuttons


Here in the screenshot i have added 10 left-margin for container and 3 left-margin for title.

rkflx added a comment.EditedDec 10 2017, 6:57 PM

Okay, so then it could work like so:

  • By default, SideBarGroupPrivate::mContainer has a margin.
  • If there should be no margin for the container (e.g. Semantic panel), this should be indicated by an optional bool flag in the SideBarGroup constructor. In this case, the caller would have to make sure to set the margins himself if required, i.e. we have to edit the .ui file here.
  • For the Folders sidebar, setting the bool would be enough.

Could you try this? To me this looks much more clean and future proof than updateTitleMargin and multiple setContentMargin calls.

rkflx added a comment.Dec 10 2017, 7:36 PM
In D9145#178033, @rkflx wrote:

I think I found a way to fix the problem properly.

Here we go, fixes submitted for review in D9281 and D9282.

Backstory how I figured it out:

Looking at the other toolbuttons, I saw they worked fine. Then I added some text to the zoom buttons in the status bar and even wrote a test app with only some buttons in different layouts, all working fine. The only explanation left was that there had to be a bug somewhere, meaning it was not Gwenview's fault. And indeed, switching styles the problem happened only with Breeze and Oxygen. But still all other toolbuttons worked fine, how could that be?

Only by chance I stumbled upon the _kde_toolbutton_alignment property in GammaRay. Tracing this one back was not straightforward, but I ended up in Breeze. Seeing the special casing for Gwenview there was quite a suprise, to say the least. One git blame, some code reading and thorough testing later the patch was ready.

In D9145#178193, @rkflx wrote:
In D9145#178033, @rkflx wrote:

I think I found a way to fix the problem properly.

Here we go, fixes submitted for review in D9281 and D9282.

Backstory how I figured it out:

Looking at the other toolbuttons, I saw they worked fine. Then I added some text to the zoom buttons in the status bar and even wrote a test app with only some buttons in different layouts, all working fine. The only explanation left was that there had to be a bug somewhere, meaning it was not Gwenview's fault. And indeed, switching styles the problem happened only with Breeze and Oxygen. But still all other toolbuttons worked fine, how could that be?

Only by chance I stumbled upon the _kde_toolbutton_alignment property in GammaRay. Tracing this one back was not straightforward, but I ended up in Breeze. Seeing the special casing for Gwenview there was quite a suprise, to say the least. One git blame, some code reading and thorough testing later the patch was ready.

That's clever :)

rkflx retitled this revision from Correct margin for Left Sidebar for gwenview. to Correct margin for left sidebar.Dec 11 2017, 11:07 PM
rkflx edited the summary of this revision. (Show Details)

Thanks so much for your patience so far, I believe we solved all hard problems and now there are only some final nitpicks and adjustments left.

app/semanticinfosidebaritem.ui
20 ↗(On Diff #23765)

In SemanticInfoContextManagerItemPrivate::setupGroup, add

formLayout->setContentsMargins(DEFAULT_LAYOUT_MARGIN, 0, 0, 0);

so you don't have to duplicate the magic number in the .ui file. Move DEFAULT_LAYOUT_MARGIN to sidebar.h for this.

38 ↗(On Diff #23765)

Removing this breaks the alignment of the rating widget.

87 ↗(On Diff #23765)

Why do you need to add that?

app/sidebar.cpp
41

Now that we indent only the text and only on the left, we can easily make this a bit wider. I think aligning "Semantic" to "Description" would be nice. I tried this and we'd need to put 6 here for this.

In addition, it looks a bit nicer for borderless windows. If you agree, please change.

app/sidebar.h
37

According to the naming convention for Qt, containerMargin and mContainerMargin for the member variable might be more readable.

49

Please move this to SideBarGroupPrivate and initialize in SideBarGroup::SideBarGroup with d->... = ....

kapillamba4 added inline comments.
app/sidebar.h
37

addContainerMargin is not a member variable, mAllowContainerMargin is.
Do you mean we should change mAllowContainerMargin to mContainerMargin? mAllowContainerMargin seems more readable i guess?

kapillamba4 marked an inline comment as done.
rkflx added inline comments.Dec 12 2017, 10:00 AM
app/sidebar.h
37

Mostly I was stumbling over the "allow" vs. "add" discrepancy, I think both should have the same name (with the member variable prefixed with m). Then I looked at the naming convention which recommends to not prefix nouns with "is/has/allow" etc.

Granted, it looks a bit like an int now. If you don't like it maybe go with something like defaultContainerMarginEnabled instead?

kapillamba4 added inline comments.Dec 12 2017, 10:02 AM
app/sidebar.h
37

How about mContainerMarginAllowed?

kapillamba4 added inline comments.Dec 12 2017, 10:04 AM
app/sidebar.h
37

nvm, defaultContainerMarginEnabled looks better

rkflx accepted this revision.EditedDec 12 2017, 10:10 AM

Latest version LGTM. Thanks a lot again for your patch.

I'll commit this on your behalf to the stable branch as soon as the freeze for 17.12.0 has ended (probably end of the week).

If you like working on Gwenview and need ideas for what to tackle next (apart from the RTL issue…), let us know ;)

This revision is now accepted and ready to land.Dec 12 2017, 10:10 AM
In D9145#178634, @rkflx wrote:

Latest version LGTM. Thanks a lot again for your patch.

Thanks for the review :)

In D9145#178634, @rkflx wrote:

If you like working on Gwenview and need ideas for what to tackle next (apart from the RTL issue…), let us know ;)

Definitely

This revision was automatically updated to reflect the committed changes.