Changeset View
Standalone View
app/sidebar.cpp
Context not available. | |||||
85 | QLabel* mTitleLabel; | 85 | QLabel* mTitleLabel; | ||
---|---|---|---|---|---|
86 | }; | 86 | }; | ||
87 | 87 | | |||
88 | SideBarGroup::SideBarGroup(const QString& title) | 88 | SideBarGroup::SideBarGroup(const QString& title, bool addContainerMargin) | ||
89 | : QFrame() | 89 | : QFrame() | ||
rkflx: That's a horrible hack which breaks `gwenview -reverse` (compare the text alignment w/ and w/o… | |||||
90 | , d(new SideBarGroupPrivate) | 90 | , d(new SideBarGroupPrivate) | ||
91 | , mAllowContainerMargin(addContainerMargin) | ||||
91 | { | 92 | { | ||
92 | d->mContainer = 0; | 93 | d->mContainer = 0; | ||
93 | d->mTitleLabel = new QLabel(this); | 94 | d->mTitleLabel = new QLabel(this); | ||
Context not available. | |||||
103 | layout->setSpacing(0); | 104 | layout->setSpacing(0); | ||
104 | 105 | | |||
105 | layout->addWidget(d->mTitleLabel); | 106 | layout->addWidget(d->mTitleLabel); | ||
107 | d->mTitleLabel->setContentsMargins(3, 0, 0, 0); | ||||
106 | 108 | | |||
107 | clear(); | 109 | clear(); | ||
108 | } | 110 | } | ||
Context not available. | |||||
142 | containerLayout->setSpacing(0); | 144 | containerLayout->setSpacing(0); | ||
143 | 145 | | |||
144 | layout()->addWidget(d->mContainer); | 146 | layout()->addWidget(d->mContainer); | ||
147 | if(mAllowContainerMargin) { | ||||
148 | d->mContainer->layout()->setContentsMargins(3, 0, 0, 0); | ||||
149 | } | ||||
145 | } | 150 | } | ||
146 | 151 | | |||
147 | void SideBarGroup::addAction(QAction* action) | 152 | void SideBarGroup::addAction(QAction* action) | ||
Context not available. | |||||
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… rkflx: This caught my eye, because setting a non-quadratic icon size seems like a hack. And indeed… | |||||
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. ngraham: Nice catch, @rkflx. It would indeed be great if we can move the icons over a bit to match the… | |||||
rkflx: Spacing now looks almost like it was designed by a professional designer, good job!
However… | |||||
See also D7581 and the API docs of drawPixmap. Maybe using the function call with ...F (floating point) parameters will help here? rkflx: See also D7581 and the API docs of `drawPixmap`. Maybe using the function call with `...F`… | |||||
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 :) rkflx: Can you remove this? I think I found a way to fix the problem properly. Still needs some… | |||||
rkflx: Phab shows a whitespace change here (added empty line), please remove. |
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.