Workaround to fix #531

[zjeffer]

This is kind of a dirty workaround to fix #531.

I have no idea how this bug happened in the first place. Somehow the issue creator had a settings file with the splittersizes line having a 0 in the QByteArray.

I'm interested to hear your opinions whether we should merge this in the first place. I have no idea if this issue will come up again, and the fix is kind of dirty.

[zjeffer]

I have no comments on the fix itself because it's a little cryptic to my knowledge

Basically, the QByteArray in the settings file is saved by m_splitter->saveState(). It is a list of 4 byte integers that looks something like this: 255, 1, 3, 222, 238, 459, 16777216, 65536, 65536. The 4th, 5th and 6th values represent the size of the splitter panes. I have no idea what the others mean.

When one of these 3 pane sizes is 0, the bug occurs (or I guess when the 4th and 5th value are, the 6th value can never be 0 because we don't hide the editor). That's why this workaround will check if one of these values is 0, and if so, assign the minimum width instead (185).

In the issue reporter's case, their QByteArray consisted of these values: 255, 1, 3, 0, 191, 1010, 16777216, 65536, 65536. It's unclear why their folder width (4th value) is 0. This should never happen, as the saveState takes into account the minimum width of the folders. So if a splitter size is 0 (which happens when a widget is hidden), saving the splitter state will save it as 185 instead of 0. This somehow didn't happen once.

[guihkx]

Thanks for the explanation. Isn't it possible that future Qt versions change how saveState() works, though? Wouldn't this change essentially bite us in the back?

[zjeffer]

Perhaps, which is why I'm hesitant to even merge this in the first place. It's likely the issue will never come up again and the "fix" might be prone to future bugs.

What do you think, @bjorn ?

[bjorn]

We can see what restoreState does by just looking at the Qt code:

https://github.com/qt/qtbase/blob/8570e86fff183ffa4c1dff577c7fa14e3342daee/src/widgets/widgets/qsplitter.cpp#L1704-L1728

So, we see apart from splitter size, it stores whether they are collapsible, the handle width, whether opaque resize is enabled and the orientation. It's rather more than I had expected to be honest, but then Qt can't make much assumptions about what state apps may want to store.

I think we should consider what state we really want to store, and consider calling only the necessary function, for example QSplitter:setSizes if the sizes are the only thing we're interested in storing (which seems likely?). Then it's also easier to apply some sanity bounds if necessary, rather then messing about in the byte array (which indeed could change with Qt version, even if it's unlikely QtWidgets will change much at this point).

[zjeffer]

Alternatively to changing what we're storing in the QSettings, as previously suggested, I guess another fix would be to check if we need to correct the sizes after calling restoreState? Or does that not address the issue we're running into?

I thought I tried this before and it didn't fix the bug, but I just tried it again and it seems to work fine now. This is indeed much simpler: simply setting the size to minimum if the width is 0 after calling restoreState.