Add support for initializer_list on arrays

[Lotendan]

Hello,

Class wxArrayString is used all throughout the project in many different components. This pull-requests intends to improve the developer experience by adding support for std::initializer_list and syntactic sugar to simplify the initialization of wxArrayString and other arrays.

Exemple:

wxArrayString choices;
choices.Add( _T( "Choice 1" ) );
choices.Add( _T( "Choice 2" ) );
wxChoice* choiceControl = new wxChoice( this, wxID_ANY, wxDefaultPosition, wxDefaultSize, choices );

// OR

wxChoice* choiceControl = new wxChoice( this, wxID_ANY );
choiceControl->Append( { _T( "Choice 1" ), _T( "Choice 2" ) } );

// Replaced by:
wxChoice* choiceControl = new wxChoice( this, wxID_ANY, wxDefaultPosition, wxDefaultSize, { _T( "Choice 1" ), _T( "Choice 2" ) } );

I wasn't sure about adding a constructor based on std::vector<std::string> or more generally std::vector<T> too. Any suggestions about this are welcome.

[vadz]

Thanks!

I definitely agree that we need to do something here, but I was thinking of doing this "something" at wxArrayStringsAdapter level, see #23036, and I'm not sure which approach is better, but they're probably mutually exclusive as we'd have ambiguous overloads if we implemented both proposals.

One advantage of this one is that it can indeed be done even in 3.2 as it's backwards compatible. And, to be honest, I don't see any problems with it.

[Lotendan]

Thank you for the first level of review, that’s really appreciated.
The issue #23036 and wxArrayStringsAdapter look interesting too, but go much further than I initially planned, especially as this is my first contribution in the repository.
Do you suggest that we still go ahead with this? At least in 3.2? Or take the time to think about it further as is pointed out in #23036? Maybe the overloads it introduces are not that much incompatible/exclusive and I see this more as a first step towards the StringsAdapter. At least the { … } syntax would remain source compatible?
Thanks again for the review :)

[vadz]

I think the main advantage of wxArrayStringsAdapter is that it could, in theory, avoid copying the strings. However it's probably not really worth it for the amounts of strings that we operate with here -- there are probably never going to be more than, what, a couple of hundreds, maybe, of them? In practice I think the worst I've ever seen was the list of US states, i.e. ~50 (without going into the details of the US federal system).

So, even after thinking about it, I don't really see any problems with doing it like this and I think we should go ahead and do it in master for now and then, if we don't find any problems in practice neither, backport it to 3.2.

Of course, the CI builds need to be fixed first -- could you please look at the errors in them? TIA!

[vadz]

Annoyingly, it looks like this doesn't compile in STL build (see the failing CI builds). I didn't yet have time to look at this, do you already see a way to fix it there?

[Lotendan]

In some cases the compiler fails to infer the template parameters in the initializer_list if T and U are not the same types (and even though T is constructible from U). I am trying another alternative to make the type inference more explicit to the compiler. With this the tests pass in local.

[Lotendan]

It worked :) The trick was to call the base constructor with iterators.
All green now! Please have one last look, and then we are ready to merge.

[vadz]

Looks good, thanks!

I'll merge this in master soon. For 3.2 we'll need to add #if wxCHECK_CXX_STD(201103L) checks around the new functions as we still support using C++98 there, but I can do it.

[Lotendan]

Sure, let me know if there's anything more I can do to help on this, or if you need me to do the PR on 3.2.

[vadz]

Sorry for completely forgetting to reply to this but a PR for 3.2 would indeed be nice.

There we still need to test whether we're using C++11 at all but, AFAICS, we don't need wxABI_VERSION checks as the functions are inline anyhow -- but it would be nice to check this, e.g. building against the latest 3.2 branch with this PR applied and verifying that the binary still runs when using distribution provided binaries for an earlier 3.2 version.

TIA!

[IreneKnapp]

Hi! I'm the nixpkgs maintainer for Tenacity, which uses wxGTK. This initializer_list work came to my attention via NixOS/nixpkgs#266945 (comment)

From preliminary investigation, it appears that this change is responsible for breaking at least Tenacity and Audacity.

I think it's a good change and should be kept. I've already prepared a fix on the Tenacity side of things and will be driving the process to get it upstreamed. Therefore, I'm not making any sort of request from you, I'm just giving you a heads up because I thought you'd like to know, in case it bears on any other planned changes or anything.

[vadz]

@IreneKnapp Thanks for reporting this and sorry for breaking it. I am not sure what exactly is the problem in {Tena,Auda}city is, but I guess there is some new overload ambiguity?

It looks like we really can't do any changes in 3.2 headers without breaking something :-( Not sure what to do about it, ideal would be to at least test the changes before the release, but we've never managed to do this. Again, I'd invite all maintainers of the packages using wx to monitor the project communications (we could create a special ultra low traffic mailing list for this or maybe post pre-release announcements to the existing wx-announce too) and help us with discovering such problems before the release. TIA!

[IreneKnapp]

yeah there was a place where it uses list initialization and it used to fall back to a copy constructor because there was no initializer list support, but now it tries to use list initialization and gets an error because the parameter it's passing is a const reference and it used to get implicitly copied but now it doesn't, so there's no appropriate method to handle it

and there was another place where it's using an overloaded assignment operator and now there's more options for what it could be so it's ambiguous, as you suggested.

I'll add a link to the Tenacity PR sometime today so you can see the details.

if you start a list like that I'll join it, I can't promise to read everything. please do let me know if us nix people can help in some way, nix is really good at rebuilding downstream stuff after changes, that has to be useful for testing somehow...

[IreneKnapp]

I'm told upstream has fixed this; anyway here's my changes that show where the problems were - NixOS/nixpkgs#266945 (comment)

[vadz]

Thanks for the explanations! I should have thought of this and kept the new ctor(s) inside some #if wxUSE_INIT_LIST... But it's too late for this now, so I'll just try to document it.

As for the list, I've asked whether a new one or posting to the existing wx-announce one would be preferable. If you have any preferences here, please let me know (here or by replying on wx-dev). For now I'm leaning towards just reusing the existing list to avoid creating too many of them.

[IreneKnapp]

no preference here, the announce list does make sense to me.

[evils]

note that it'd be nice if i could subscribe to the announcements without having to involve a google account
(this seems to work for the KiCad dev mailing list / google group, though maybe that only works when it's in read+write)

[vadz]

@evils I don't think you need a Google account to subscribe to the mailing list, at least not if you do it via email, i.e. send a request to wx-announce+subscribe@googlegroups.com. If this doesn't work for you, please contact me and we'll try to figure this out (although dealing with Google groups is difficult...).

[evils]

@vadz
i did send a mail there before sending my message here
i just confirmed again, it doesn't work
i do see a "contact owners and managers" button, which pops up a message authoring window, but its content fails to load
so either something is blocking its content, or it also requires a google account

reviewing the KiCad devlist subscription email, maybe i did link it to my google account...
though neither of my google accounts show it is "my groups"

joining does have an option to uncheck "link to my google profile"
but that does add the group to my google groups "my groups" list (and seems to just reuse my avatar image)
and didn't send a join confirmation to the email address i attempted to subscribe from
(my main issue there is which inbox the mails go to, aside from requiring more than an email address to subscribe...)

[vadz]

@evils Unfortunately I can confirm that it doesn't work, I've tested it by posting an email to subscribe from a unique address and got a reply directing me to https://groups.google.com/group/wx-announce/subscribe where I can't do anything. I don't see anything I can do about this in the group options neither, unfortunately.

I can only suggest that you contact me by email and I try adding your email to the list directly, I think this might work but I'm not even sure about any more. Sorry.

[swt2c]

I can only suggest that you contact me by email and I try adding your email to the list directly, I think this might work but I'm not even sure about any more. Sorry.

That definitely does work. I maintain a Google Group for something unrelated and the administrative interface does allow you to add non-Google users manually. They users can't access the web interface (eg, archives) though.