GNOME Bugzilla – Bug 700205
Save as..., a way to recover the initial file name
Last modified: 2018-05-02 15:39:50 UTC
When saving a file, the dialog as a default filename. We can browse the file system If we click on a file, the default name is replaced by this file's name (meaning that we want to overwrite that file) But now, it happen that we click on a file by mistake. (Especially it happens regularly to me that I click one too many times on a folder, then the third click goes to the file "behind" that folder). This erase the default filename. And you can't get it back. Currently the only way is to cancel the file selection, and start again going through a folder path (which might be somehow complex). ==> add a way to recover the initial file name. (maybe after the name is changed, a button icon could appear in the name's text entry, and would restore the initial name on activate?)
I'd gladly review a patch to do that (make a button appear in the entry after it gets changed through selecting another file).
I can give it a try
Created attachment 247874 [details] [review] 0001--bgo700205--Save-as--a-way-to-recover-the-initial-file-name--0.1.patch a first patch which should work in the ACTION_SAVE case I am unsure if this is hooked correctly with the various use cases. NOTES: - it is build with 3.8.1 (I don't manage to build 3.9 on my machine) - I left printerr lines to track what happens - all lines prefixed by "/**/" are meant to be removed - there are a few "FIXME" to be reviewed - testfilechooser seems to use ACTION_OPEN what-so-ever modified: gtk/gtkfilechooserdefault.c modified: gtk/gtkfilechooserentry.c modified: gtk/gtkfilechooserentry.h modified: gtk/gtkfilechooserprivate.h modified: tests/testfilechooser.c --- gtk/gtkfilechooserdefault.c | 7 +++- gtk/gtkfilechooserentry.c | 88 ++++++++++++++++++++++++++++++++++++++++++++- gtk/gtkfilechooserentry.h | 3 +- gtk/gtkfilechooserprivate.h | 1 + tests/testfilechooser.c | 5 +++ 5 files changed, 101 insertions(+), 3 deletions(-)
Created attachment 248085 [details] [review] patch update I reworked the patch. Now it should cover all the use cases for file save and folder create. It is against 3.8.2
Created attachment 248309 [details] [review] 0001--bgo700205--Save-as--a-way-to-recover-the-initial-file-name--0.3.patch update: fix the null string bug I am now using this patch on my machine.
Created attachment 250799 [details] [review] 0001--bgo700205--Save-as--a-way-to-recover-the-initial-file-name--0.4.patch ported to master/HEAD (9579386)
Federico, could you have a look?
Review of attachment 250799 [details] [review]: First I have some comments about commit log. While I must appreciate that you took the time to write such a detailed commit log, I don't think log is for explaining every C change in English. This would fall under the "over-documentation" category. So I'd suggest: * Summarize the change * Tell about potential issues (if any) * Keep the short log under 50 characters & each line in details under 74 characters as per conventions (see https://wiki.gnome.org/Git/CommitMessages ). * W/ shortlog preferred to be short, there is no room for redundant characters like '...,' & no marks needed at the end. Also please rebase your change against git master of gtk+. 3.8 is about to be unsupported and only critical fixes go to that. ::: gtk/gtkfilechooserdefault.c @@ +4784,3 @@ + /* save file basename, to be able to restore it (on undo) */ + char *s = NULL; + s = g_file_get_basename (file); /**/g_printerr ("oluc> s after -->%s<-- %d\n", s, strlen (s)); These lines go beyond 80 chars so not only they don't follow coding-style, I can't read them in splinter here. @@ +7635,3 @@ + +const gchar * +_gtk_file_chooser_get_location_initial_entry (GtkFileChooserDefault *chooser){ coding-style: { on next line. ::: gtk/gtkfilechooserentry.c @@ +33,3 @@ #include "gtkwindow.h" #include "gtkintl.h" +#include "deprecated/gtkstock.h" /* FIXME */ We definitely don't wan to use deprecated stock API. @@ +237,3 @@ + /* Set a clickable icon to reset the filename to its initial value */ + //Commented out "if" test: ./tests/testfilechooser uses ACTION_OPEN, whatsoever... Its obvious that its commented out and you want to put a comprehensive comment here. At least its not comprehensible for my feeble brain. :)
(In reply to comment #8) > Review of attachment 250799 [details] [review]: > > First I have some comments about commit log. While I must appreciate that you > took the time to write such a detailed commit log, I don't think log is for > explaining every C change in English. This would fall under the > "over-documentation" category. So I'd suggest: > > * Summarize the change > * Tell about potential issues (if any) > * Keep the short log under 50 characters & each line in details under 74 > characters as per conventions (see https://wiki.gnome.org/Git/CommitMessages ). > * W/ shortlog preferred to be short, there is no room for redundant characters > like '...,' & no marks needed at the end. OK, I will do > Also please rebase your change against git master of gtk+. 3.8 is about to be > unsupported and only critical fixes go to that. my last patch is against git master, or did I miss something? > > ::: gtk/gtkfilechooserdefault.c > @@ +4784,3 @@ > + /* save file basename, to be able to restore it (on undo) */ > + char *s = NULL; > + s = g_file_get_basename (file); > /**/g_printerr ("oluc> s after -->%s<-- %d\n", s, strlen (s)); > > These lines go beyond 80 chars so not only they don't follow coding-style, I > can't read them in splinter here. yes, /**/ until EOL is meant to be removed in the final patch. It helps me while I am still working on the patch. I will remove them. > @@ +7635,3 @@ > + > +const gchar * > +_gtk_file_chooser_get_location_initial_entry (GtkFileChooserDefault *chooser){ > > coding-style: { on next line. OK! > ::: gtk/gtkfilechooserentry.c > @@ +33,3 @@ > #include "gtkwindow.h" > #include "gtkintl.h" > +#include "deprecated/gtkstock.h" /* FIXME */ > > We definitely don't wan to use deprecated stock API. Yes. Here I am unsure what to use. Should it be a stock icon or a symbolic icon. In both cases I need to figure out how to do it. I will work it out. > @@ +237,3 @@ > > + /* Set a clickable icon to reset the filename to its initial value */ > + //Commented out "if" test: ./tests/testfilechooser uses ACTION_OPEN, > whatsoever... > > Its obvious that its commented out and you want to put a comprehensive comment > here. At least its not comprehensible for my feeble brain. :) The commented out lines are meant to be active in the final patch. But with those lines, you can't try the patch with testfilechooser. I did not manage to fix testfilechooser itself, so I commented out these lines. I will remove the comment, and activate the lines Thanks!
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 250799 [details] [review] [details]: > > > > First I have some comments about commit log. While I must appreciate that you > > took the time to write such a detailed commit log, I don't think log is for > > explaining every C change in English. This would fall under the > > "over-documentation" category. So I'd suggest: > > > > * Summarize the change > > * Tell about potential issues (if any) > > * Keep the short log under 50 characters & each line in details under 74 > > characters as per conventions (see https://wiki.gnome.org/Git/CommitMessages ). > > * W/ shortlog preferred to be short, there is no room for redundant characters > > like '...,' & no marks needed at the end. > > OK, I will do > > > > Also please rebase your change against git master of gtk+. 3.8 is about to be > > unsupported and only critical fixes go to that. > > my last patch is against git master, or did I miss something? No, actually *I* misread. :) Sorry. > > ::: gtk/gtkfilechooserdefault.c > > @@ +4784,3 @@ > > + /* save file basename, to be able to restore it (on undo) */ > > + char *s = NULL; > > + s = g_file_get_basename (file); > > /**/g_printerr ("oluc> s after -->%s<-- %d\n", s, strlen (s)); > > > > These lines go beyond 80 chars so not only they don't follow coding-style, I > > can't read them in splinter here. > > yes, /**/ until EOL is meant to be removed in the final patch. It helps me > while I am still working on the patch. I will remove them. Yes please. Alternatively you can put them in a form and language that helps others too so everyone is happy, including reviewers. :) > > ::: gtk/gtkfilechooserentry.c > > @@ +33,3 @@ > > #include "gtkwindow.h" > > #include "gtkintl.h" > > +#include "deprecated/gtkstock.h" /* FIXME */ > > > > We definitely don't wan to use deprecated stock API. > > Yes. Here I am unsure what to use. Should it be a stock icon or a symbolic > icon. In both cases I need to figure out how to do it. I will work it out. The docs for each GTK_STOCK_* you tell you the replacement: https://developer.gnome.org/gtk3/3.9/gtk3-Stock-Items.html#GTK-STOCK-ABOUT:CAPS > > @@ +237,3 @@ > > > > + /* Set a clickable icon to reset the filename to its initial value */ > > + //Commented out "if" test: ./tests/testfilechooser uses ACTION_OPEN, > > whatsoever... > > > > Its obvious that its commented out and you want to put a comprehensive comment > > here. At least its not comprehensible for my feeble brain. :) > > The commented out lines are meant to be active in the final patch. But with > those lines, you can't try the patch with testfilechooser. I did not manage to > fix testfilechooser itself, so I commented out these lines. > > I will remove the comment, and activate the lines And please also fix testfilechooser. That would be expected as part of this patch.
I assume this file chooser is what Firefox uses. The file name can be lost not just to selecting a local file, but even to simple text editing of the field. So can the usual text Undo, including Ctrl+Z, be added? An annoying instance of this bug is a race condition when downloading many files using Firefox (not counting people who would make a script parsing the page). Example: 1. You need to save multiple files from the Web, enough in quantity or size to push the connection limit, so that you do not get all Save As dialogs immediately on clicking the link or "Save Link As". 2. You don't want to wait for each file chooser separately, so you click many links at a time (or type/paste the URLs), to then wait a little for the file choosers to appear. 3. File chooser appears under the pointer when you are about to (left-)click, and you hit an existing local file, replacing the initial file name. Or you are typing/pasting at the moment, and replace the name. Now you don't know what the file was, which file name will be missing. The files are not named regularly. Maybe you know you have clicked too much and will need to skip one of the duplicates (did you just enter that _correct_ target name for that file and only then noticed that there was already one on disk?). Maybe you feel you've missed a link and will have to look through the names to find which one you need to click again, but actually you'd be downloading it again.
Note: I am not working on this anymore and I am afraid that there is very little chance that I come back to it in the near or far future. So, anybody, feel free to catch the ball! As far as I recall, I think that I got it right. It just need to be updated to current gtk, and perform the changes suggested by Zeeshan's good review.
i would like to take up this bug. started initially by incorporating these changes on my local machine.
(In reply to Adarsh from comment #13) > i would like to take up this bug. go ahead Adarsh! You're warmly welcome!
Created attachment 305279 [details] [review] Migrated the changes as per 3.17.3 code base. incorporated the review comments. I have added the changes mentioned in bugzilla and did a simple round of testing and it seems to work fine. even the deprecated functions are changed to latest. No new features are added.
Review of attachment 305279 [details] [review]: test
(In reply to Adarsh from comment #15) > Created attachment 305279 [details] [review] [review] > Migrated the changes as per 3.17.3 code base. incorporated the review > comments. > > I have added the changes mentioned in bugzilla and did a simple round of > testing and it seems to work fine. even the deprecated functions are changed > to latest. No new features are added. I did a review but there is currently some bug that is not letting me publish it. To get the feedback to you fast, I'll attach screenshots.
Created attachment 305284 [details] Screenshot of review #1
Created attachment 305285 [details] Screenshot of review #2
(about the commented-out "if", see my last point in comment #9)
(In reply to Luc Pi from comment #20) > (about the commented-out "if", see my last point in comment #9) Ah ok. Its been a while. :) They shouldn't be in the patch itself but an additional patch so they don't accidently end up in git master.
Created attachment 305330 [details] [review] Code cleanup and other comments incorporated. Latest changes committed after unit-testing.
Review of attachment 305330 [details] [review]: The commit log is * completely ignoring all the guidelines I pointed to: https://wiki.gnome.org/Git/CommitMessages * talking of changes to patch rather than describing the changes in the patch (its good that you mentioned them but you want to add them in the comment when attaching the patch rather than in the commit log itself). I'm a bit confused too. Is this patch on top of the previous patch? If so, please squash them since the previous patch is not obsolete now (which you should mark it as such). Talking of which, whatever happened to my comment about dividing the two logical changes into separate independent patches? ::: gtk/gtkfilechooserentry.c @@ -34,3 @@ #include "gtkwindow.h" #include "gtkintl.h" -//#include "deprecated/gtkstock.h" Please remove this. ::: gtk/gtkfilechooserprivate.h @@ +105,3 @@ GError **error); GSList * _gtk_file_chooser_list_shortcut_folder_files (GtkFileChooser *chooser); +gchar * _gtk_file_chooser_get_location_initial_entry (GtkFileChooserWidget *chooser); I think you're expected to alight the function names and arguments with other funciton declarations around it. ::: gtk/gtkfilechooserwidget.c @@ -25,2 @@ #include "config.h" - redundant change? Before submitting patch, please do a quick review to ensure everything is ok. @@ -27,2 @@ #include "gtkfilechooserwidget.h" - same here. @@ -626,2 @@ unset_file_system_backend (impl); - and here. @@ -664,2 @@ impl->priv = NULL; - and here. Seems there is many of these. ::: gtk/gtkfilechooserwidget.h @@ +69,3 @@ GtkWidget *gtk_file_chooser_widget_new (GtkFileChooserAction action); +//const gchar * _gtk_file_chooser_get_location_initial_entry (GtkFileChooserWidget *chooser); ?????
(In reply to Zeeshan Ali (Khattak) from comment #23) > Review of attachment 305330 [details] [review] [review]: > > The commit log is > > * completely ignoring all the guidelines I pointed to: > https://wiki.gnome.org/Git/CommitMessages > * talking of changes to patch rather than describing the changes in the > patch (its good that you mentioned them but you want to add them in the > comment when attaching the patch rather than in the commit log itself). will add proper messages to commit log. > > I'm a bit confused too. Is this patch on top of the previous patch? If so, > please squash them since the previous patch is not obsolete now (which you > should mark it as such). Talking of which, whatever happened to my comment > about dividing the two logical changes into separate independent patches? > What will the 2 independent patches contain ? I am a bit unclear on this. I have migrated the code changes to 3.17.3 code base and incorporating the review changes. > ::: gtk/gtkfilechooserentry.c > @@ -34,3 @@ > #include "gtkwindow.h" > #include "gtkintl.h" > -//#include "deprecated/gtkstock.h" > > Please remove this. > > ::: gtk/gtkfilechooserprivate.h > @@ +105,3 @@ > GError **error); > GSList * _gtk_file_chooser_list_shortcut_folder_files (GtkFileChooser > *chooser); > +gchar * _gtk_file_chooser_get_location_initial_entry (GtkFileChooserWidget > *chooser); > > I think you're expected to alight the function names and arguments with > other funciton declarations around it. > > ::: gtk/gtkfilechooserwidget.c > @@ -25,2 @@ > #include "config.h" > - > > redundant change? Before submitting patch, please do a quick review to > ensure everything is ok. > > @@ -27,2 @@ > #include "gtkfilechooserwidget.h" > - > > same here. > > @@ -626,2 @@ > unset_file_system_backend (impl); > - > > and here. > > @@ -664,2 @@ > impl->priv = NULL; > - > > and here. Seems there is many of these. > > ::: gtk/gtkfilechooserwidget.h > @@ +69,3 @@ > GtkWidget *gtk_file_chooser_widget_new (GtkFileChooserAction > action); > > +//const gchar * _gtk_file_chooser_get_location_initial_entry > (GtkFileChooserWidget *chooser); > > ????? will remove the redundant new lines. My apologies for the previous ccommit. there are many empty lines.
> > I'm a bit confused too. Is this patch on top of the previous patch? If so, > > please squash them since the previous patch is not obsolete now (which you > > should mark it as such). Talking of which, whatever happened to my comment > > about dividing the two logical changes into separate independent patches? > > What will the 2 independent patches contain ? I am a bit unclear on this. I > have migrated the code changes to 3.17.3 code base and incorporating the > review changes. your patch should include _all_ the needed change for the feature (and _only_ the needed changes: do not touch other lines). About your latest attachment, it seems that it's just changes on top of a previous patch. You should include _all_ the changes in _one_ patch, based on git current master (maybe that's 3.17.3). Zeeshan mentioned a second patch for the code that I changed in order to test the feature (like the g_printerr statements, and the commented-out "if" statement), because this code is useful for testing, but it must absolutely not go in the final commit. So testers can apply both patches, and the final committer will commit only the first one. The commit message should describe the feature. What it does, why and eventually how it does it. (see the changelog guidelines)
Created attachment 305452 [details] [review] Migrated the changes as per 3.17.3 code base. incorporated the review comments. will create another independent patch after removing g_printerr etc.. once this patch is base lined. Currently incorporated the changes mentioned.
(In reply to Luc Pi from comment #25) > > > I'm a bit confused too. Is this patch on top of the previous patch? If so, > > > please squash them since the previous patch is not obsolete now (which you > > > should mark it as such). Talking of which, whatever happened to my comment > > > about dividing the two logical changes into separate independent patches? > > > > What will the 2 independent patches contain ? I am a bit unclear on this. I > > have migrated the code changes to 3.17.3 code base and incorporating the > > review changes. > > > your patch should include _all_ the needed change for the feature > (and _only_ the needed changes: do not touch other lines). No, that is wrong! The rule is "Every logical change goes into its own patch". So I really meant dividing the patch into two (three if including the private g_print and comments etc).
Review of attachment 305452 [details] [review]: Thanks for updating the patch and it looks much better already but: 1. Please see my previous comment here. Each patch is for a logical change, not feature. 2. Your shortlog/summary and description breaks the rule about max number of chars per line. Please compare against the checklist on https://wiki.gnome.org/Git/CommitMessages ::: gtk/gtkfilechooserentry.c @@ -19,2 @@ #include "config.h" - still unrelated changes here? @@ -22,3 @@ - -#include <string.h> - Why is this header inclusion getting removed? @@ +201,3 @@ + + /* Selecting the file/folder if it exists, otherwise clear selection */ + gchar *path = NULL; variable declarations on the top only. @@ +235,3 @@ g_object_set (chooser_entry, "truncate-multiline", TRUE, NULL); + //Set a clickable icon to reset the filename to its initial value Single line (//) comments are not welcome as per coding-style AFAIK. You want to use /* */ comments. @@ +238,3 @@ + if (chooser_entry->action == GTK_FILE_CHOOSER_ACTION_SAVE || + chooser_entry->action == GTK_FILE_CHOOSER_ACTION_CREATE_FOLDER|| + chooser_entry->action == GTK_FILE_CHOOSER_ACTION_OPEN) Align please. @@ -229,3 @@ { GtkFileChooserEntry *chooser_entry = GTK_FILE_CHOOSER_ENTRY (object); - Seems your patch still introduces a bunch of these redundant line removal and addition changes. Could you please do a through review of each change in your patch?
> > your patch should include _all_ the needed change for the feature > > (and _only_ the needed changes: do not touch other lines). > > No, that is wrong! The rule is "Every logical change goes into its own > patch". (sorry for this bad information, my bad.)
Created attachment 305546 [details] [review] Basic User interface added into initial patch. Stock items have been removed since they are deprecated from version 3.10 Added commit-comments as per the standards. Divided the initial patch into 2 sets. This patch contains the user interface where an undo button is displayed. All the Signals are not handled in this patch. Redundant changes in the code have been removed and spacing between the code is handled.
Created attachment 305547 [details] [review] Patch contains handling of signals on click of the button. This is add-on patch on the previous patch. Various conditions on click of the button handled and review comments incorporated.
Created attachment 305584 [details] [review] Removed the private g_print messages. Incorpoarted the review comments Allignnment and declaration of variable comments of previous review rectified. Private g_print_err messages are removed.
(In reply to Luc Pi from comment #0) > When saving a file, the dialog as a default filename. > We can browse the file system > > If we click on a file, the default name is replaced by this file's name > (meaning that we want to overwrite that file) > > But now, > it happen that we click on a file by mistake. (Especially it happens > regularly to me that I click one too many times on a folder, then the third > click goes to the file "behind" that folder). This erase the default > filename. And you can't get it back. Currently the only way is to cancel the > file selection, and start again going through a folder path (which might be > somehow complex). > > > ==> add a way to recover the initial file name. > > > > (maybe after the name is changed, a button icon could appear in the name's > text entry, and would restore the initial name on activate?) you can try by copying the entire content to a new file and saving that file with a different name .
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
I believe this would still be a good thing to have. Though I am not able to work on this anymore. At some point it was working. I don't know how the master branch evolved since. Maybe not that much ? Feel free to continue on... (I can only change the status between NEEDINFO and RESOLVED...)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/425.