Differential D15625 Diff 42003 kdevplatform/sublime/ideallayout.cpp

Changeset View

Standalone View

View Options

kdevplatform/sublime/ideallayout.cpp

Show First 20 Lines • Show All 46 Lines • ▼ Show 20 Line(s)43{
47 QLayout::invalidate(); 47​ QLayout::invalidate();
48} 48​}
49 49​
50IdealButtonBarLayout::~IdealButtonBarLayout() 50​IdealButtonBarLayout::~IdealButtonBarLayout()
51{ 51​{
52 qDeleteAll(_items); 52​ qDeleteAll(_items);
53} 53​}
54 54​
55void IdealButtonBarLayout::setHeight(int height) 55​void IdealButtonBarLayout::setHeight(int height)
kossebauUnsubmitted
Done

IdealButtonBarLayout::~IdealButtonBarLayout() = default; please

kossebau: `IdealButtonBarLayout::~IdealButtonBarLayout() = default;` please
56{ 56​{
57 Q_ASSERT(orientation() == Qt::Vertical); 57​ Q_ASSERT(orientation() == Qt::Vertical);
58 _height = height; 58​ _height = height;
59 59​
60 (void) invalidate(); 60​ (void) invalidate();
61} 61​}
kossebauUnsubmitted
Done

We have an issue here. buttonSpacing() looks for any parentWidget() to take the style from, otherwise defaults to 0.
And currently we create IdealButtonBarLayout without any QWidget parent, so at this point in time the spacing is set to the default value 0.
So we need to somehow learn about the containing parent widget.

One solution might be to add an explicit property *QWidget* m_styleParentWidget` to
IdealButtonBarLayout, which gets set via the constructor, so it is available from the start. And could also be used for catching the event about style changes.

kossebau: We have an issue here. `buttonSpacing()` looks for any `parentWidget()` to take the style from…
62 62​
63Qt::Orientation IdealButtonBarLayout::orientation() const 63​Qt::Orientation IdealButtonBarLayout::orientation() const
64{ 64​{
65 return _orientation; 65​ return _orientation;
66} 66​}
67 67​
68Qt::Orientations IdealButtonBarLayout::expandingDirections() const 68​Qt::Orientations IdealButtonBarLayout::expandingDirections() const
69{ 69​{
Show All 22 Lines74{
92 return m_min; 92​ return m_min;
93} 93​}
94 94​
95QSize IdealButtonBarLayout::sizeHint() const 95​QSize IdealButtonBarLayout::sizeHint() const
96{ 96​{
97 if (m_sizeHintDirty) { 97​ if (m_sizeHintDirty) {
98 const int buttonSpacing = this->buttonSpacing(); 98​ const int buttonSpacing = this->buttonSpacing();
99 99​
100 int orientationSize = 0; 100​ int orientationSize = 0;
kossebauUnsubmitted
Not Done

While this is inspired by the sizeHint() method, I wonder if we should not rather do a qMax()over all crossSizes. Just to be safe against someone starting to add a non-IdealToolButton one day. Not that expensive to do.
If agreed, in a later separate commit we could do the same for sizeHint().

kossebau: While this is inspired by the `sizeHint()` method, I wonder if we should not rather do a `qMax…
101 int crossSize = 0; 101​ int crossSize = 0;
102 102​
103 bool first = true; 103​ bool first = true;
104 for (QLayoutItem *item : _items) { 104​ for (QLayoutItem *item : _items) {
105 QSize hint = item->sizeHint(); 105​ QSize hint = item->sizeHint();
106 int orientationSizeHere; 106​ int orientationSizeHere;
107 int crossSizeHere; 107​ int crossSizeHere;
108 if (orientation() == Qt::Vertical) 108​ if (orientation() == Qt::Vertical)
▲ Show 20 Lines • Show All 88 Lines • ▼ Show 20 Line(s)192{
197 int x = rect.x() + l; 197​ int x = rect.x() + l;
198 int y = rect.y() + t; 198​ int y = rect.y() + t;
199 int currentLineWidth = 0; 199​ int currentLineWidth = 0;
200 200​
201 if (_items.empty()) { 201​ if (_items.empty()) {
202 return x + currentLineWidth + r; 202​ return x + currentLineWidth + r;
203 } 203​ }
204 204​
205 const bool shrink = rect.height() < sizeHint().height(); 205​ bool shrink = rect.height() < sizeHint().height();
206 206​
207 const int maximumHeight = rect.height() / _items.size(); 207​ // space left per button after removing available space for buttonSpacing
208​ const int maximumHeight = (rect.height() - buttonSpacing * (_items.size() - 1)) / _items.size();
208 int shrinkedHeight = -1; 209​ int shrinkedHeight = -1;
209 210​
210 if (shrink) { 211​ if (shrink) {
211 int smallItemCount = 0; 212​ int smallItemCount = 0;
212 const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItemCount](int acc, QLayoutItem* item) { 213​ const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItemCount](int acc, QLayoutItem* item) {
213 const int itemHeight = item->sizeHint().height(); 214​ const int itemHeight = item->sizeHint().height();
214 if (itemHeight <= maximumHeight) { 215​ if (itemHeight <= maximumHeight) {
215 acc += maximumHeight - itemHeight; 216​ acc += maximumHeight - itemHeight;
216 ++smallItemCount; 217​ ++smallItemCount;
217 } 218​ }
218 return acc; 219​ return acc;
219 }); 220​ });
220 221​
221 Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width 222​ if (_items.size() != smallItemCount) {
kossebauUnsubmitted
Done

Please add a comment that this is protective code against the chance there is some bug in our logic which would lead to divide-by-zero then. Also worth a qCWarning in the else branch, so we could collect info where this happens.

Given the calculations above, there should be at least one too-large item. Otherwise we would have a bug in our sizeHint() code or in the what-and-how-to-shrink code above. Or the layouted items would suddenly start to report random sizeHint() values during the execution of this method, which would be even more unexpected.

kossebau: Please add a comment that this is protective code against the chance there is some bug in our…
222 // evenly distribute surplus height over large items 223​ // evenly distribute surplus height over large items
223 shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount); 224​ shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount);
225​ } else {
226​ // only small items, no shrinkage needed
227​ shrink = false;
228​ }
224 } 229​ }
225 230​
226 for (QLayoutItem* item : _items) { 231​ for (QLayoutItem* item : _items) {
227 const QSize itemSizeHint = item->sizeHint(); 232​ const QSize itemSizeHint = item->sizeHint();
228 const int itemWidth = itemSizeHint.width(); 233​ const int itemWidth = itemSizeHint.width();
229 int itemHeight = itemSizeHint.height(); 234​ int itemHeight = itemSizeHint.height();
230 235​
231 if (shrink && itemSizeHint.height() > maximumHeight) { 236​ if (shrink && itemSizeHint.height() > maximumHeight) {
Show All 9 Lines
241 y += itemHeight + buttonSpacing; 246​ y += itemHeight + buttonSpacing;
242 } 247​ }
243 248​
244 m_layoutDirty = updateGeometry; 249​ m_layoutDirty = updateGeometry;
245 250​
246 return x + currentLineWidth + r; 251​ return x + currentLineWidth + r;
247} 252​}
248 253​
249int IdealButtonBarLayout::doHorizontalLayout(const QRect &rect, bool updateGeometry) const 254​int IdealButtonBarLayout::doHorizontalLayout(const QRect &rect, bool updateGeometry) const
kossebauUnsubmitted
Done

eventFilter reimplementation best also call the one of the base class, as one never knows if the base classes do not need to do some filtering as well. Might not be the case here currently, but let's be future-proof and base class co-operative by default :)

Usually done by doing in the final return statement, that also spares a variable to remember the result (also drops the need for Q_UNUSED()):

return QBoxLayout::eventFilter(watched, event);
kossebau: eventFilter reimplementation best also call the one of the base class, as one never knows if…
250{ 255​{
251 const int buttonSpacing = this->buttonSpacing(); 256​ const int buttonSpacing = this->buttonSpacing();
252 257​
253 int l, t, r, b; 258​ int l, t, r, b;
254 getContentsMargins(&l, &t, &r, &b); 259​ getContentsMargins(&l, &t, &r, &b);
255 int x = rect.x() + l; 260​ int x = rect.x() + l;
256 int y = rect.y() + t; 261​ int y = rect.y() + t;
257 int currentLineHeight = 0; 262​ int currentLineHeight = 0;
258 263​
259 if (_items.empty()) { 264​ if (_items.empty()) {
260 return y + currentLineHeight + b; 265​ return y + currentLineHeight + b;
261 } 266​ }
262 267​
263 const bool shrink = rect.width() < sizeHint().width(); 268​ bool shrink = rect.width() < sizeHint().width();
264 269​
265 const int maximumWidth = rect.width() / _items.size(); 270​ // space left per button after removing available space for buttonSpacing
271​ const int maximumWidth = (rect.width() - buttonSpacing * (_items.size() - 1)) / _items.size();
266 int shrinkedWidth = -1; 272​ int shrinkedWidth = -1;
267 273​
268 if (shrink) { 274​ if (shrink) {
269 int smallItemCount = 0; 275​ int smallItemCount = 0;
270 const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItemCount](int acc, QLayoutItem* item) { 276​ const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItemCount](int acc, QLayoutItem* item) {
271 const int itemWidth = item->sizeHint().width(); 277​ const int itemWidth = item->sizeHint().width();
272 if (itemWidth <= maximumWidth) { 278​ if (itemWidth <= maximumWidth) {
273 acc += maximumWidth - itemWidth; 279​ acc += maximumWidth - itemWidth;
274 ++smallItemCount; 280​ ++smallItemCount;
275 } 281​ }
276 return acc; 282​ return acc;
277 }); 283​ });
278 284​
279 Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width 285​ if (_items.size() != smallItemCount) {
kossebauUnsubmitted
Done

Same comment please.

kossebau: Same comment please.
280 // evenly distribute surplus width on the large items 286​ // evenly distribute surplus width on the large items
281 shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount); 287​ shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount);
288​ } else {
289​ shrink = false;
290​ }
kossebauUnsubmitted
Not Done

Wait, this should be the inverse: "Expected at least one too-large item" :)
Same above.

kossebau: Wait, this should be the inverse: "Expected at least one too-large item" :) Same above.
rjvbbUnsubmitted
Not Done

Please make this a qCDebug; no need to bother the user with this information because it will be printed really lots of times if the situation arises.

rjvbb: Please make this a qCDebug; no need to bother the user with this information because it will be…
282 } 291​ }
283 292​
284 for (QLayoutItem* item : _items) { 293​ for (QLayoutItem* item : _items) {
285 const QSize itemSizeHint = item->sizeHint(); 294​ const QSize itemSizeHint = item->sizeHint();
286 int itemWidth = itemSizeHint.width(); 295​ int itemWidth = itemSizeHint.width();
287 const int itemHeight = itemSizeHint.height(); 296​ const int itemHeight = itemSizeHint.height();
288 297​
289 if (shrink && itemSizeHint.width() > maximumWidth) { 298​ if (shrink && itemSizeHint.width() > maximumWidth) {
Show All 17 Lines