Differential D15625 Diff 42268 kdevplatform/sublime/ideallayout.cpp

Changeset View

Standalone View

View Options

kdevplatform/sublime/ideallayout.cpp

Show All 16 Lines1/*
17 FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE 17​ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
18 KDEVELOP TEAM BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN 18​ KDEVELOP TEAM BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
19 AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN 19​ AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
20 CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. 20​ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
21*/ 21​*/
22 22​
23#include "ideallayout.h" 23​#include "ideallayout.h"
24 24​
25​#include <debug.h>
26​
25#include <QStyle> 27​#include <QStyle>
26#include <QWidget> 28​#include <QWidget>
27 29​
28#include <numeric> 30​#include <numeric>
29 31​
30using namespace Sublime; 32​using namespace Sublime;
31 33​
32IdealButtonBarLayout::IdealButtonBarLayout(Qt::Orientation orientation, QWidget *parent) 34​IdealButtonBarLayout::IdealButtonBarLayout(Qt::Orientation orientation, QWidget *parent)
Show All 14 Lines45{
47 QLayout::invalidate(); 49​ QLayout::invalidate();
48} 50​}
49 51​
50IdealButtonBarLayout::~IdealButtonBarLayout() 52​IdealButtonBarLayout::~IdealButtonBarLayout()
51{ 53​{
52 qDeleteAll(_items); 54​ qDeleteAll(_items);
53} 55​}
54 56​
55void IdealButtonBarLayout::setHeight(int height) 57​void IdealButtonBarLayout::setHeight(int height)
kossebauUnsubmitted
Done

IdealButtonBarLayout::~IdealButtonBarLayout() = default; please

kossebau: `IdealButtonBarLayout::~IdealButtonBarLayout() = default;` please
56{ 58​{
57 Q_ASSERT(orientation() == Qt::Vertical); 59​ Q_ASSERT(orientation() == Qt::Vertical);
58 _height = height; 60​ _height = height;
59 61​
60 (void) invalidate(); 62​ (void) invalidate();
61} 63​}
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 64​
63Qt::Orientation IdealButtonBarLayout::orientation() const 65​Qt::Orientation IdealButtonBarLayout::orientation() const
64{ 66​{
65 return _orientation; 67​ return _orientation;
66} 68​}
67 69​
68Qt::Orientations IdealButtonBarLayout::expandingDirections() const 70​Qt::Orientations IdealButtonBarLayout::expandingDirections() const
69{ 71​{
70 return orientation(); 72​ return orientation();
71} 73​}
72 74​
73QSize IdealButtonBarLayout::minimumSize() const 75​QSize IdealButtonBarLayout::minimumSize() const
74{ 76​{
75 // The code below appears to be completely wrong -- ​
76 // it will return the maximum size of a single button, not any ​
77 // estimate as to how much space is necessary to draw all buttons ​
78 // in a minimally acceptable way. ​
79 if (m_minSizeDirty) { 77​ if (m_minSizeDirty) {
78​ const int buttonSpacing = this->buttonSpacing();
79​
80​ bool first = true;
81​ int orientationSize = 0;
82​ int crossSize = 0;
83​ for (QLayoutItem* item : _items) {
84​ const QSize itemMinSize = item->minimumSize();
85​
86​ int orientationSizeHere;
87​ int crossSizeHere;
80 if (orientation() == Qt::Vertical) { 88​ if (orientation() == Qt::Vertical) {
81 const int width = doVerticalLayout(QRect(0, 0, 0, _height), false); 89​ orientationSizeHere = itemMinSize.height();
82 return QSize(width, 0); 90​ crossSizeHere = itemMinSize.width();
91​ }
92​ else {
93​ orientationSizeHere = itemMinSize.width();
94​ crossSizeHere = itemMinSize.height();
83 } 95​ }
84 96​
85 m_min = QSize(0, 0); 97​ if (first) {
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…
86 for (QLayoutItem* item : _items) { 98​ crossSize = crossSizeHere;
87 m_min = m_min.expandedTo(item->minimumSize()); 99​ first = false;
100​ }
101​ else {
102​ orientationSize += buttonSpacing;
103​ }
104​ orientationSize += orientationSizeHere;
105​ }
106​ if (orientation() == Qt::Vertical) {
107​ m_min = QSize(crossSize, orientationSize);
108​ } else {
109​ m_min = QSize(orientationSize, crossSize);
88 } 110​ }
89 111​
90 m_minSizeDirty = false; 112​ m_minSizeDirty = false;
91 } 113​ }
92 return m_min; 114​ return m_min;
93} 115​}
94 116​
95QSize IdealButtonBarLayout::sizeHint() const 117​QSize IdealButtonBarLayout::sizeHint() const
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Line(s)214{
197 int x = rect.x() + l; 219​ int x = rect.x() + l;
198 int y = rect.y() + t; 220​ int y = rect.y() + t;
199 int currentLineWidth = 0; 221​ int currentLineWidth = 0;
200 222​
201 if (_items.empty()) { 223​ if (_items.empty()) {
202 return x + currentLineWidth + r; 224​ return x + currentLineWidth + r;
203 } 225​ }
204 226​
205 const bool shrink = rect.height() < sizeHint().height(); 227​ bool shrink = rect.height() < sizeHint().height();
206 228​
207 const int maximumHeight = rect.height() / _items.size(); 229​ // space left per button after removing available space for buttonSpacing
230​ const int maximumHeight = (rect.height() - buttonSpacing * (_items.size() - 1)) / _items.size();
208 int shrinkedHeight = -1; 231​ int shrinkedHeight = -1;
209 232​
210 if (shrink) { 233​ if (shrink) {
211 int smallItemCount = 0; 234​ int smallItemCount = 0;
212 const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItemCount](int acc, QLayoutItem* item) { 235​ const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItemCount](int acc, QLayoutItem* item) {
213 const int itemHeight = item->sizeHint().height(); 236​ const int itemHeight = item->sizeHint().height();
214 if (itemHeight <= maximumHeight) { 237​ if (itemHeight <= maximumHeight) {
215 acc += maximumHeight - itemHeight; 238​ acc += maximumHeight - itemHeight;
216 ++smallItemCount; 239​ ++smallItemCount;
217 } 240​ }
218 return acc; 241​ return acc;
219 }); 242​ });
220 243​
221 Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width 244​ // Sanity check to prevent against a divide-by-zero due to some latent miscalculations
245​ if (_items.size() != smallItemCount) {
222 // evenly distribute surplus height over large items 246​ // evenly distribute surplus height over large items
223 shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount); 247​ shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount);
248​ } else {
249​ qCWarning(SUBLIME) << "Expected at least one large item, none found, possible erraneous shrink predicate or sizeHint";
250​ shrink = false;
251​ }
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…
224 } 252​ }
225 253​
226 for (QLayoutItem* item : _items) { 254​ for (QLayoutItem* item : _items) {
227 const QSize itemSizeHint = item->sizeHint(); 255​ const QSize itemSizeHint = item->sizeHint();
228 const int itemWidth = itemSizeHint.width(); 256​ const int itemWidth = itemSizeHint.width();
229 int itemHeight = itemSizeHint.height(); 257​ int itemHeight = itemSizeHint.height();
230 258​
231 if (shrink && itemSizeHint.height() > maximumHeight) { 259​ if (shrink && itemSizeHint.height() > maximumHeight) {
Show All 9 Lines
241 y += itemHeight + buttonSpacing; 269​ y += itemHeight + buttonSpacing;
242 } 270​ }
243 271​
244 m_layoutDirty = updateGeometry; 272​ m_layoutDirty = updateGeometry;
245 273​
246 return x + currentLineWidth + r; 274​ return x + currentLineWidth + r;
247} 275​}
248 276​
249int IdealButtonBarLayout::doHorizontalLayout(const QRect &rect, bool updateGeometry) const 277​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{ 278​{
251 const int buttonSpacing = this->buttonSpacing(); 279​ const int buttonSpacing = this->buttonSpacing();
252 280​
253 int l, t, r, b; 281​ int l, t, r, b;
254 getContentsMargins(&l, &t, &r, &b); 282​ getContentsMargins(&l, &t, &r, &b);
255 int x = rect.x() + l; 283​ int x = rect.x() + l;
256 int y = rect.y() + t; 284​ int y = rect.y() + t;
257 int currentLineHeight = 0; 285​ int currentLineHeight = 0;
258 286​
259 if (_items.empty()) { 287​ if (_items.empty()) {
260 return y + currentLineHeight + b; 288​ return y + currentLineHeight + b;
261 } 289​ }
262 290​
263 const bool shrink = rect.width() < sizeHint().width(); 291​ bool shrink = rect.width() < sizeHint().width();
264 292​
265 const int maximumWidth = rect.width() / _items.size(); 293​ // space left per button after removing available space for buttonSpacing
294​ const int maximumWidth = (rect.width() - buttonSpacing * (_items.size() - 1)) / _items.size();
266 int shrinkedWidth = -1; 295​ int shrinkedWidth = -1;
267 296​
268 if (shrink) { 297​ if (shrink) {
269 int smallItemCount = 0; 298​ int smallItemCount = 0;
270 const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItemCount](int acc, QLayoutItem* item) { 299​ const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItemCount](int acc, QLayoutItem* item) {
271 const int itemWidth = item->sizeHint().width(); 300​ const int itemWidth = item->sizeHint().width();
272 if (itemWidth <= maximumWidth) { 301​ if (itemWidth <= maximumWidth) {
273 acc += maximumWidth - itemWidth; 302​ acc += maximumWidth - itemWidth;
274 ++smallItemCount; 303​ ++smallItemCount;
275 } 304​ }
276 return acc; 305​ return acc;
277 }); 306​ });
278 307​
279 Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width 308​ // Sanity check to prevent against a divide-by-zero due to some latent miscalculations
309​ if (_items.size() != smallItemCount) {
280 // evenly distribute surplus width on the large items 310​ // evenly distribute surplus width on the large items
281 shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount); 311​ shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount);
312​ } else {
313​ qCDebug(SUBLIME) << "Expected at least one large item, none found, possible erraneous shrink predicate or sizeHint";
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…
314​ shrink = false;
315​ }
kossebauUnsubmitted
Done

Same comment please.

kossebau: Same comment please.
282 } 316​ }
283 317​
284 for (QLayoutItem* item : _items) { 318​ for (QLayoutItem* item : _items) {
285 const QSize itemSizeHint = item->sizeHint(); 319​ const QSize itemSizeHint = item->sizeHint();
286 int itemWidth = itemSizeHint.width(); 320​ int itemWidth = itemSizeHint.width();
287 const int itemHeight = itemSizeHint.height(); 321​ const int itemHeight = itemSizeHint.height();
288 322​
289 if (shrink && itemSizeHint.width() > maximumWidth) { 323​ if (shrink && itemSizeHint.width() > maximumWidth) {
Show All 17 Lines