Ask for forgiveness when closing scratchpads, don't make me click annoying "don't save" buttons

ASSIGNED
Assigned to

Status

P3
normal
ASSIGNED
6 years ago
3 months ago

People

(Reporter: fitzgen, Assigned: darkowlzz, Mentored)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We should add an easy undo and reopen the last scratchpad you had open instead of forcing people to respond to a prompt every time they close a scratchpad.

Possibly add an entry to the recently opened files or something.
I'd like that, too. We used to treat scratchpads that had never been saved as files differently in the past, which is I think the behavior we want.
Duplicate of this bug: 883246
Priority: -- → P3
(Assignee)

Comment 3

3 years ago
Created attachment 8613697 [details] [diff] [review]
[WIP]  Added Scratchpad option to open last unsaved file

Hey, I have done some work on this issue and here is a patch of my WIP. I need some feedback on the patch.

This patch adds a `Open Last Unsaved Scratchpad` option under File Menu. It also disables the "Don't Save?" prompt when close button is clicked. On close, Scratchpad checks if the file is already saved (by looking at the filename), if the file isn't saved and asked to close, it would save the file as recently unsaved file. If the file is saved, it would just close the file.

Once a file is unsaved and closed, clicking on File > Open Last Unsaved Scratchpad
would open the closed file.

The unsaved file is being saved by Scratchpad in the user's profile directory with the name "recentScratchpad.txt". This file is overwritten when there is a new unsaved close.


This is just a draft, would polish and refine it later. I need some feedback if this is the right way to do it.

Thanks!
Attachment #8613697 - Flags: feedback?(past)
Attachment #8613697 - Flags: feedback?(nfitzgerald)
Comment on attachment 8613697 [details] [diff] [review]
[WIP]  Added Scratchpad option to open last unsaved file

Review of attachment 8613697 [details] [diff] [review]:
-----------------------------------------------------------------

I'm afraid this patch throws out the baby with the bathwater. It should still prompt to save changes to previously saved files, as that is the main case that bug 794898 tried to fix. In fact I'd like to revert the change in that bug to promptSave.

Apart from that, storing a previously unsaved scratchpad to a file is great and I like it. I originally wanted opening a new scratchpad to just open any previously unsaved scratchpad, like vim/emacs. This approach is also good, but it's less discoverable.
Attachment #8613697 - Flags: feedback?(past)
(Assignee)

Comment 5

3 years ago
Created attachment 8614310 [details] [diff] [review]
[WIP] Added Scratchpad option to open last unsaved file

Some more progress.

Baby out of the water ;)

I can still sense that this would affect other parts of scratchpad. I would run the tests and fix those affected parts once we get this feature right.

Looks good? What more can be done?
Please suggest better name for the file menu option and recently unsaved scratchpad file if they doesn't feel right.

How about handling the discover-ability in another bug?
Attachment #8613697 - Attachment is obsolete: true
Attachment #8613697 - Flags: feedback?(nfitzgerald)
Attachment #8614310 - Flags: feedback?(past)
Comment on attachment 8614310 [details] [diff] [review]
[WIP] Added Scratchpad option to open last unsaved file

Review of attachment 8614310 [details] [diff] [review]:
-----------------------------------------------------------------

This does feel much better. One thing that is awkward is that the moment you load the last unsaved scratchpad it becomes a file-backed one. So further changes to it are stored to the hidden file in the profile directory. What I would have expected is that loading the last unsaved scratchpad would still appear as a not-file-backed scratchpad, unless I explicitly saved it in a path I provided. This could be done by dumping the contents of the temporary file into the editor on load, and then letting the rest of the code do its thing.

I think I'm warming up to the "open last unsaved" menu item approach versus always loading it on startup.

::: browser/devtools/scratchpad/scratchpad.js
@@ +39,5 @@
>  const EDITOR_FONT_SIZE = "devtools.scratchpad.editorFontSize";
>  const ENABLE_AUTOCOMPLETION = "devtools.scratchpad.enableAutocompletion";
>  const TAB_SIZE = "devtools.editor.tabsize";
>  const FALLBACK_CHARSET_LIST = "intl.fallbackCharsetList.ISO-8859-1";
> +const RECENT_SCRATCHPAD = "recentScratchpad.txt";

I think lastUnsavedScratchpad.txt would be easier to find if one were to look for it. It also has a more direct association with the menu item.

@@ +1266,5 @@
>      }
>    },
>  
> +  openLastUnsaved: function SP_openLastUnsaved() {
> +    let file = FileUtils.getFile("ProfD", [RECENT_SCRATCHPAD]);

Scratchpad needs to be converted to use OS.File for better performance. Try to use that instead of FileUtils here.

::: browser/devtools/scratchpad/scratchpad.xul
@@ +164,5 @@
>          <menupopup id="sp-menu-open_recentPopup"/>
>        </menu>
>  
> +      <menuitem id="sp-open_last-unsaved"
> +                label="Open Last Unsaved Scratchpad"

Open Last Unsaved should be enough. We are inside Scratchpad after all. Also, this needs to go in scratchpad.dtd.
Attachment #8614310 - Flags: feedback?(past) → feedback+
Assignee: nobody → indiasuny000
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
Created attachment 8616759 [details] [diff] [review]
Added Scratchpad option to open last unsaved file

Now the the unsaved scratchpad is loaded without being backed by a file.

I need help in deciding a proper access key and command key for the menu item "Open Last Unsaved".
Attachment #8614310 - Attachment is obsolete: true
Attachment #8616759 - Flags: feedback?(past)
Comment on attachment 8616759 [details] [diff] [review]
Added Scratchpad option to open last unsaved file

Review of attachment 8616759 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't play with the patch this time, but I think it's ready for proper review if you add a test.

::: browser/devtools/scratchpad/scratchpad.js
@@ +39,5 @@
>  const EDITOR_FONT_SIZE = "devtools.scratchpad.editorFontSize";
>  const ENABLE_AUTOCOMPLETION = "devtools.scratchpad.enableAutocompletion";
>  const TAB_SIZE = "devtools.editor.tabsize";
>  const FALLBACK_CHARSET_LIST = "intl.fallbackCharsetList.ISO-8859-1";
> +const LAST_UNSAVED_SCRATCHPAD = "lastUnsavedScratchpad.txt";

fitzgen suggested using "unsaved-scratchpad-1.txt", for future compatibility, in case we ever decide to support multiple unsaved scratchpads. That sounds like a good idea to me.

::: browser/locales/en-US/chrome/browser/devtools/scratchpad.dtd
@@ +36,5 @@
>  <!ENTITY openRecentMenu.label         "Open Recent">
>  <!ENTITY openRecentMenu.accesskey     "R">
>  
> +<!ENTITY openUnsaved.label            "Open Last Unsaved">
> +<!ENTITY openUnsaved.accesskey        "U">

This seems like a good access key, as it doesn't conflict with the others in the File menu. A command key is not strictly necessary, but I think we can use Cmd-Shift-O (Ctrl-Shift-O) or Cmd-Opt-O (Ctrl-Alt-O). Alt and Shift have historically been used to slightly alter the effect of a command and in this case we are opening a file (which is Cmd-O/Ctrl-O), but a very specific one.
Attachment #8616759 - Flags: feedback?(past) → feedback+

Comment 9

3 years ago
I actually find this prompt mechanism very useful. Am I right that you're going to implement for scratchpad windows simplified "Open last closed" mechanism that is now used for browser windows?
 1) I just wanted to warn you that if it will be simplified (e.g. "Open only 1 last closed window",
    then user may lost data in Scratchpad window
 2) Also, I see you're going to make Ctrl+U shortcut, but I thought it's logical to make it 
    Ctrl+Shift+N, just like shortcut to restore browser windows
 3) Currently all opened Scratchpad windows are being silently saved on shutdown, when you click 
    (≡)->"Exit Nightly". So the same could be used for cases when user closes last browser window
    in a normal way (see bug 1203250)
Blocks: 1203250

Updated

3 years ago
Blocks: 1203277

Updated

3 years ago
No longer blocks: 1203277

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.