OSX fixing file dialog with only one filter (PR #26151)
Stefan Csomor
see #26148
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26151
Commit Summary
- 69d6962 fixing default file filter
File Changes
(1 file)
- M src/osx/cocoa/filedlg.mm (8)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26151.patch
- https://github.com/wxWidgets/wxWidgets/pull/26151.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -287,6 +287,10 @@ - (void)setAllowedExtensions:(const wxArrayString &)extensions
{
m_firstFileTypeFilter = GetMatchingFilterExtension(m_fileName);
}
+ else
+ {
+ m_firstFileTypeFilter = 0;
Shouldn't we just initialize it to 0 above instead? This would be more clear IMHO.
Note that GetMatchingFilterExtension() already returns 0 instead of -1 if there is no match.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -287,6 +287,10 @@ - (void)setAllowedExtensions:(const wxArrayString &)extensions
{
m_firstFileTypeFilter = GetMatchingFilterExtension(m_fileName);
}
+ else
+ {
+ m_firstFileTypeFilter = 0;
If you agree, I can just make this change myself and push it to both branches.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> @@ -607,6 +611,10 @@ - (void)setAllowedExtensions:(const wxArrayString &)extensions
{
m_firstFileTypeFilter = GetMatchingFilterExtension(m_fileName);
}
+ else
It would also be great to refactor this code, at least in master, to avoid having big chunks of same code in both wxFileDialog::ShowWindowModal() and ShowModal()...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
@csomor commented on this pull request.
> @@ -287,6 +287,10 @@ - (void)setAllowedExtensions:(const wxArrayString &)extensions
{
m_firstFileTypeFilter = GetMatchingFilterExtension(m_fileName);
}
+ else
+ {
+ m_firstFileTypeFilter = 0;
yes that would be fine, thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26151)@csomor Sorry, just noticed that after this change this code
doesn't make sense any more as m_firstFileTypeFilter is always positive or zero.
Does it mean that we should set it to 0 only when saving? AFAICS this test is more or less equivalent to m_useFileTypeFilter but I don't fully understand what's going on here :-(
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26151)I'll clean up
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
@csomor pushed 1 commit.
- 934f419 moving code to common method
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks a lot!
If this is the final version, I'll merge it and backport to 3.2 (adding non-virtual private member function should be ok even there).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26151)Thanks a lot!
If this is the final version, I'll merge it and backport to 3.2 (adding non-virtual private member function should be ok even there).
Yes, I think it's done. You're very welcome - the code duplication irked me in the past as well - so it was a good moment ... and in a way to make sure we can backport it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26151)I've pushed this as 7c6300a and 17dbee8 and then added 5b96b76 to try to clarify things a bit. As I wrote there, it's still not clear to me what does setting m_filterExtensions to -1 achieve, but at least the code works without asserts/crashes now.
If you don't see any problems with what I did, I'll backport at least 17dbee8 to 3.2 too.
Thanks again!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Richard
nobugshere left a comment (wxWidgets/wxWidgets#26151)I'm not sure if it's related to these changes, but I tested it from current master and the save files dialog does not give me a default file name extension nor the drop-down list to select from the options. (dialogs sample, Dialogs->File Operations->Save file) It works as expected in the 3.2 branch.
macOS 15.7.3/Xcode 16.4
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26151)@nobugshere thanks I'll test
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Stefan Csomor
csomor left a comment (wxWidgets/wxWidgets#26151)reproduced, I'll add a new PR
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.