Recent macOS regression after wxCOL_WIDTH_AUTOSIZE changes for wxDataViewCtrl

[wxtrac]

Issue migrated from trac ticket # 19067

component: wxOSX | priority: normal | resolution: fixed | keywords: regression wxDataViewCtrl

2021-01-29 14:16:39: @discnl created the issue

---

Since the changes in 7555d1b245 (with compilable version since 2a4c52a414) there's a problem with some columns. To reproduce:

1. Start the dataview sample
2. Select the second tab, "MyListModel"

column 0 (using wxDataViewCheckIconTextRenderer) and column 3 (using wxDataViewTextRenderer) are very narrow and seem to have minimal width (columns can still be resized though), and column 1 (a plain text column) has clipped content. All three columns use wxCOL_WIDTH_AUTOSIZE. Before they were wide enough, with column 1 not being ellipsized.

[wxtrac]

2021-02-05 22:45:32: @softwarefailure commented

---

Ok, this was a little tricky but the attached patch should fix it. The problem was that we didn't call AdjustAutosizedColumns() when columns were added or removed but of course AdjustAutosizedColumns() must also be called in that case because adding/removing columns will affect column widths.

The problem when calling AdjustAutosizedColumns() when adding or inserting columns, however, is that columns might temporarily become the last column which will always be given the remaining space. This doesn't make sense however in case the column is only temporarily the last column. But since we can't tell at insertion time whether this column will be the last one temporarily or permanently we also have to store its current width before we give it all the remaining space. If it's only temporarily the last column, we'll restore its width as soon as a new column becomes the last one.

I've tested my fix with the dataview sample and your minimal sample from #14939 and it seems to work correctly so I think this can be applied.

[wxtrac]

2021-02-05 22:45:55: @softwarefailure uploaded file mypatch.patch (3.9 KiB)

[wxtrac]

2021-02-06 18:24:30: @discnl changed status from new to confirmed

2021-02-06 18:24:30: @discnl commented

Thanks a lot for fixing this! Confirmed those regressions are gone and also did some testing again with wxCOL_WIDTH_AUTOSIZE on the first page of the dataview sample and comparing to 3.1.4, looks nice :). I see some oddity on the "MyListModel" page where after making the first column quite wide and then ticking the box (which resizes the column...) the last column becomes squashed. This already also happened with 3.1.4 however and is not related to wxCOL_WIDTH_AUTOSIZE changes.

Vadim: This only appears to fix recent regressions and not break things. I don't know if it's final (see below) but is it therefore already OK to commit soonish rather than it lingering around? If you would prefer to do it, e.g. as part of a branch collecting wxOSX changes, then please go ahead of course.

Replying to [comment:1 afalkenhahn]:

The problem when calling AdjustAutosizedColumns() when adding or inserting columns, however, is that columns might temporarily become the last column which will always be given the remaining space. This doesn't make sense however in case the column is only temporarily the last column. But since we can't tell at insertion time whether this column will be the last one temporarily or permanently we also have to store its current width before we give it all the remaining space. If it's only temporarily the last column, we'll restore its width as soon as a new column becomes the last one.

That's annoying to deal with. Without knowing the mechanics at play: is it because of something wx does (e.g. adjust a new column's index only later on) or is it coming from the OS? In the latter case: perhaps there are other/more (later) notifications that could be helpful?

Oh, and since this is the third time I'm seeing it now I have to ask: where does m_ExpanderWidth (with capital "E", it should be m_expanderWidth) come from in your recent patches? :) Are your changes retrofitted into a clean wx clone and a patch created from there or something? Otherwise I don't know what's going on :).

[wxtrac]

2021-02-06 18:40:48: @softwarefailure commented

---

Replying to [comment:3 disc]:

That's annoying to deal with. Without knowing the mechanics at play: is it because of something wx does (e.g. adjust a new column's index only later on) or is it coming from the OS? In the latter case: perhaps there are other/more (later) notifications that could be helpful?

I think the basic problem is that AFAICS columns can be added and removed at any time. If the API was something like

dv->BeginColumnChange();
dv->AppendColumn();
dv->FinishColumnChange();

Then it would be much easier because we could just refit columns on FinishColumnChange(). With the API as it is, however, we never know when a column is inserted if it's going to be the last one permanently or if it's only temporarily the last one because the code will soon add another one.

Oh, and since this is the third time I'm seeing it now I have to ask: where does m_ExpanderWidth (with capital "E", it should be m_expanderWidth) come from in your recent patches? :) Are your changes retrofitted into a clean wx clone and a patch created from there or something? Otherwise I don't know what's going on :).

Yes, I have a separate version of wxWidgets I'm working with because typically not all of my patches make it into release versions. So I always retrofit my patches into the latest master but I don't check if they compile because compiling wxWidgets takes an hour here on my old Mac Mini without SSD. And I don't use git or anything.

[wxtrac]

2021-02-06 18:42:08: @vadz commented

---

Yes, I'm all for committing this, but I need to find time to test it as I don't want to break the build again as I did the last time I committed a patch with m_ExpanderWidth without testing.

The last column problem is due to our API, you always (well, usually) add columns in order, so each of them becomes the last one. Normally the solution would be to postpone calling AdjustAutosizedColumns(), e.g. by just using CallAfter(), but I'm not sure if this isn't going to create more problems and I don't have time to do and test this anyhow, so unless somebody else does it, we'll have to live with this solution, even if storing the last widths really shouldn't be necessary.

[wxtrac]

2021-02-09 00:00:01: @discnl commented

---

Replying to [comment:5 vadz]:

Yes, I'm all for committing this, but I need to find time to test it as I don't want to break the build again as I did the last time I committed a patch with m_ExpanderWidth without testing.

Sure, thanks. I'm at least attaching my diff to today's master (cleaned up for error/whitespace/some formatting, left alone otherwise).

The last column problem is due to our API, you always (well, usually) add columns in order, so each of them becomes the last one. Normally the solution would be to postpone calling AdjustAutosizedColumns(), e.g. by just using CallAfter(), but I'm not sure if this isn't going to create more problems and I don't have time to do and test this anyhow, so unless somebody else does it, we'll have to live with this solution, even if storing the last widths really shouldn't be necessary.

In combination with the analysis from Andreas, using (in some way) CallAfter() may also be promising. I wanted to play around with CallAfter() in master and then noticed that for fixing this ticket only the two AdjustAutosizedColumns() changes seem to be needed (well strictly speaking only the first one).

Andreas, as you describe it the ticket's problem should still be there when only adding the AdjustAutosizedColumns() changes from the patch. Unfortunately, for me then it already works. Is it still not working for you in that case? I also tried prepending and inserting columns in the dataview sample (modified to use wxCOL_WIDTH_AUTOSIZE columns on the first page) but wasn't able to force a problem that way either.

Replying to [comment:4 afalkenhahn]:

Yes, I have a separate version of wxWidgets I'm working with because typically not all of my patches make it into release versions. So I always retrofit my patches into the latest master but I don't check if they compile because compiling wxWidgets takes an hour here on my old Mac Mini without SSD. And I don't use git or anything.

Makes sense, thanks for explaining. And that seems like a long time for anything Intel. If that's configure-based, are you using multiple jobs with e.g. make -j4 already? What about configuring with something like: --disable-html --disable-xrc --disable-aui --disable-propgrid --disable-ribbon --disable-stc --disable-richtext --disable-webview --disable-webrequest

? This reduces the compilation time by well over 1/3 here.

Also there's --enable-pch. If you configure using that then your builds would be slower than without pre-compiled headers (see also 9f4f075034). There's an ugly kludge modifying a Makefile to get PCH working with wx for clang (C++ and ObjC++). Run makefile-pch-conversion.sh in a dir having a Makefile configured with --enable-pch (without it will currently bork the Makefile). Depending on job count and machine it reduces the compilation time by 31-35% here (all full plain configure builds without options).

[wxtrac]

2021-02-09 00:00:47: @discnl uploaded file 19067-master.diff (3.7 KiB)

[wxtrac]

2021-02-10 13:24:22: @softwarefailure commented

---

Replying to [comment:6 disc]:

Andreas, as you describe it the ticket's problem should still be there when only adding the AdjustAutosizedColumns() changes from the patch. Unfortunately, for me then it already works. Is it still not working for you in that case? I also tried prepending and inserting columns in the dataview sample (modified to use wxCOL_WIDTH_AUTOSIZE columns on the first page) but wasn't able to force a problem that way either.

Just adding AdjustAutosizedColumns() to InsertColumn() will indeed solve the problem you described but it will introduce all sorts of new problems. It was the first thing I tried and then the problem you described was indeed gone but I remember other pages in the dataview sample getting messed up so I had to add the isLast kludge.

Makes sense, thanks for explaining. And that seems like a long time for anything Intel. If that's configure-based, are you using multiple jobs with e.g. make -j4 already? What about configuring with something like: --disable-html --disable-xrc --disable-aui --disable-propgrid --disable-ribbon --disable-stc --disable-richtext --disable-webview --disable-webrequest

? This reduces the compilation time by well over 1/3 here.

Ok, I'll try but generally the machine is very slow. It's a 2012 Mac Mini running 10.13 and it got slower with each new macOS so I finally stopped installing OS updates at 10.13 :)

[wxtrac]

2021-02-27 11:56:49: @vadz commented

---

@disc Do you know what is the status of this in the latest master? I'm afraid I got completely lost among all the various patches here, if you know what remains to be done here, I'd really appreciate if you could please make a PR with the changes. TIA!

[wxtrac]

2021-04-09 18:35:39: @vadz changed status from confirmed to closed

2021-04-09 18:35:39: @vadz set resolution to fixed

2021-04-09 18:35:39: @vadz commented

This should be fixed by a02690f957 (Merge branch 'mac-dvc-autosize', 2021-04-08).