docs: mark "Show hidden files" file dialog setting as deprecated on Linux#46926
Conversation
|
💖 Thanks for opening this pull request! 💖 Semantic PR titlesWe use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Commit signingThis repo enforces commit signatures for all incoming PRs. PR tipsThings that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
|
Thanks for starting this change, additional context in #34706 (comment). As discussed before, to make this a |
|
@deepak1556 I read that comment before. GtkFileChooser* chooser = gtk_file_chooser_dialog_new(...);
gtk_file_chooser_get_show_hidden(chooser); // always returns false
gtk_file_chooser_set_show_hidden(chooser, true);
gtk_file_chooser_get_show_hidden(chooser); // true
gtk_file_chooser_set_show_hidden(chooser, false);
gtk_file_chooser_get_show_hidden(chooser); // falseIt must be emphasized that the current code does nothing but breaks user experience. I don't see any useful effects from it. Therefore its removal should be considered as a semver patch. Moreover, as I said before, I can't test my code because my computer can't handle |
|
I was finally able to compile Electron. I can confirm that this patch works. |
6f863e7 to
00e99b9
Compare
|
I made a rebase to resolve conflicts |
There was a problem hiding this comment.
@zonescape's right -- #46817 is a valid bug -- but I don't think we should take this patch as-is.
This patch does honor the user's default settings, but it stops honoring any showHiddenFiles value passed into the options.properties: string[] in dialog.showOpenDialogSync() et al. on Linux.
I think the root bug is that Electron's Linux code unconditionally calls SetHiddenShown() whether or not showHiddenFiles was present in options.properties. This causes a system preference of "show hidden files" to get clobbered if options.properties is empty.
IMO we should not change the patch file. Instead, @zonescape WDYT about making this change instead:
diff --git a/shell/browser/ui/file_dialog_linux.cc b/shell/browser/ui/file_dialog_linux.cc
index 56251d9717..1ddd7c34dd 100644
--- a/shell/browser/ui/file_dialog_linux.cc
+++ b/shell/browser/ui/file_dialog_linux.cc
@@ -197,7 +197,8 @@ class FileChooserDialog : public ui::SelectFileDialog::Listener {
int hidden_flag = type_ == DialogType::SAVE
? static_cast<int>(SAVE_DIALOG_SHOW_HIDDEN_FILES)
: static_cast<int>(OPEN_DIALOG_SHOW_HIDDEN_FILES);
- dialog_->SetHiddenShown(settings.properties & hidden_flag);
+ if (settings.properties & hidden_flag)
+ dialog_->SetHiddenShown(true);
}
DialogType type_;00e99b9 to
dde52ab
Compare
|
@ckerr your code doesn't change anything because void SelectFileDialogLinuxGtk::GtkFileChooserSetShowHidden(GtkFileChooser* chooser) {
if (!show_hidden()) {
// partial fix for gtk_file_chooser_set_show_hidden()
// overwriting the system value of the is-hidden property
return;
}
gtk_file_chooser_set_show_hidden(chooser, show_hidden()); // overwriting
}But I don't understand why users who run Electron apps with As far as I understand, Electron's "show hidden files" feature can not be implemented for GTK3 dialogs. It literally can not be implemented. Whatever we do will break the user experience. Therefore I don't understand what we are trying to achieve? To break user expierience in half? By a quarter? Why? Electron apps are infamous for their non native look and behaviour. This issue is a perfect example of non native behaviour. I think this should be avoided as much as possible. As for app developers, I propose to add corresponding docs (see code). |
This feature was added before my time, so I'd have to use the commit history the same as you to know why this feature exists. It's because it was requested in #3262 and implemented in #6431. From the screenshot included in 3262, it looks like one use case at the time was for using a file chooser to select ssh keys that could be located in hidden directories like So nobody in this PR was there at the time, but feel free to throw abuse at me anyway. If you'd like to continue in this vein, I can step away from this PR and maybe another maintainer will work with you.
If by this you mean it's unavoidable that we let apps do this without clobbering the user settings -- I think that's technically not true. We could work around this by caching the user's preference before opening the chooser, and then restoring it as part of chooser teardown. But if by this you mean "no app on GNOME desktops should have this feature" then I think I agree. And on just a pragmatic level, this feature will be removed sooner or later because |
There was a problem hiding this comment.
Removing support for this feature on GNOME / *nix is a breaking change, so we need to deprecate it now and then remove it after the deprecation period.
-
In docs use the annotations
_macOS_,_Windows_,_Linux_to say what platforms a feature works on. We use the annotation_Deprecated_to mark deprecated API. We don't seem to have something for API deprecated only on one platform, so my inline suggestions of_macOS_ _Windows_ _Deprecated+ the comment "Deprecated on Linux" is probably close enough. -
Needs a comment in https://github.com/electron/electron/blob/main/docs/breaking-changes.md#planned-breaking-api-changes-400 along the lines of
### Deprecated: `showHiddenFiles` in Dialogs on Linux
This property will still be honored on macOS and Windows, but support on Linux
will be removed in Electron 41. GTK intends for this to be a user choice rather
than an app choice and has removed the API to do this programmatically.- Don't change
feat_add_support_for_missing_dialog_features_to_shell_dialogs.patchin this PR. That will have to wait for after the deprecation period.
|
@ckerr done |
f09a025 to
4a97eda
Compare
|
(rebased to |
jkleinsc
left a comment
There was a problem hiding this comment.
Given where we are in the release cycle, I believe it is too late to mark this as deprecated in 40 and removed in 41. We should mark it as deprecated for 41 and remove in a future release.
4a97eda to
7d3a4eb
Compare
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
Release Notes Persisted
|
|
I have automatically backported this PR to "41-x-y", please check out #49948 |
…inux (electron#46926) * fix: don't overwrite "Show hidden files" setting on Linux/GTK * docs: deprecate showHiddenFiles property in dialogs on Linux * docs: mark Electron 42 as the removal date for this feature --------- Co-authored-by: Charles Kerr <charles@charleskerr.com>
Description of Change
Closes #46817
GTK file chooser doesn't need special setup regarding hidden files. Current code just overwrites system wide user preferences.
As far as I understand showHiddenFiles property is supposed to be application wide. If this is correct then this property can't be implemented for GTK dialogs because they read/write system wide "Show hidden files" setting.
This is a draft PR because I can't check my code. My computer hangs ongclient sync --with_branch_heads --with_tagscommand from these instructions. Typing in terminal doesn't work, mouse is not moving, I can't even use tty3 because of login timeout. Switched off the computer. I hope somebody can test my solution.@ckerr @codebytere @deepak1556
Checklist
npm testpassesRelease Notes
Notes: Marked "Show hidden files" setting as deprecated on Linux/GNOME