New file menu item - Reopen Closed Tab

squarebat commented 2 years ago

As discussed in Issue #508 , this PR implements the functionality for reopening previously closed tabs. It has been added to the file menu after Open Require Path... as 'Reopen Closed Tab'. Since Ctrl-Shift-T is reserved for 'Spell Check Test' in the edit menu, I gave this the shortcut Ctrl-*, which might be a bit unintuitive.

As in @Metaxal 's quickscript, the callback reopen-closed-tab finds the first file in recently-opened-files that isn't already open and still exists. If no tabs were closed in the current session, it will open files that were active in previous sessions. However, after the recently-opened-files list is exhausted, it will just open new tabs. I thought it would be best for the callback to reside in unit.rkt where related methods create-new-tab and open-in-new-tab are stored.

This has only been manually tested, how should I write the tests for it? I couldn't find an appropriate file in the repository for testing GUI related features.

I will admit I couldn't look into this issue for a long time, and I am extremely sorry about the delay ;-;

The corresponding PR to add the new string constant reopen-closed-tab - string-constants/pull/56

rfindler commented 2 years ago

Thanks for putting some time into this!

I don't think that using 'framework:recently-opened-files/pos will work. That's not only recently closed files, but it is updated at other times too. I think this needs to be a new preference.

Also, since \<menukey>-shift-t is so widely used for this shortcut, I wouldn't mind changing that.

squarebat commented 2 years ago

@rfindler on futher exploring, recently-opened-files does behave unexpectedly for this feature. I'll set up a new preference that maintains a list of closed tabs.

rfindler commented 2 years ago

Great! The preference will probably need to be initialized and maintained in the racket/gui repo (in the framework, near to where 'framework:recently-opened-files/pos is maintained).

squarebat commented 2 years ago

I have added a new preference recently-closed-tabs. This list is updated whenever a tab is closed, and also when a tab is reopened. It works as expected but it is still incomplete with respect to :

As in gui/PR257, the default test and un/marshall functions for recently-closed-tabs are the same as recently-opened-files. Should we create named procs for the lambdas used?
In update-closed-tabs, I want to remove redundant tabs and introduce a size limit for the list. This should ideally be done by the add-to-recent method in gui-lib/framework/private/handler.rkt . However it currently handles only recently-opened-files. I tried to create an abstraction (add-to-list) and add another method add-to-closed-tabs. But I couldn't install it correctly and can't access the method outside handler. This might be because I am testing my code on a slightly older version. I'll update and check again.
The shortcut for Spell Check Test needs to be updated since -shift-t is now reserved for reopen closed tab.

rfindler commented 2 years ago

I think that since the framework doesn't seem to be notified when a tab is closed that maybe it makes the most sense to have this preference managed entirely in drracket itself (so not in the framework). So I'd suggest moving the preference creation and initialization into drracket/private/main and just having one pull request.
I think the preference contain only be files that aren't open. So reopen-closed-tab can simply open the first one in the list. When opening a file another way, however, it would need to be removed from the list. This is probably not how browsers work, tho? I'm not sure. Maybe trying an experiment to see what happens when you close a tab and then go visit the same url on your own and then try to reopen it?
It does make sense to put a limit on the number of files in the preference. A simple way way to do that is to add the new file to the front of list and then check how long the list becomes. If it is too long, then remove things from the end. Something like this:

#lang racket

(define (truncate-list l n)
  (define len (length l))
  (cond
    [(<= len n) l]
    [else (drop-right l (- len n))]))

(require rackunit)
(check-equal? (truncate-list '(1 2 3) 10) '(1 2 3))
(check-equal? (truncate-list '(1 2 3) 3) '(1 2 3))
(check-equal? (truncate-list '(1 2 3) 2) '(1 2))
(check-equal? (truncate-list '(1 2 3) 1) '(1))
(check-equal? (truncate-list '(1 2 3 4 5 6) 2) '(1 2))

The two numbers in the preference (that go along with the filename) are meant to be the insertion position. These can be recovered by calling get-editor or get-definitions-text and then asking for its get-start-position and get-end-position and saving them in the preference.
There is the issue comparing paths to determine if this is "the same file" or not (when opening a file, that file name might need to be removed from the preference). I think probably using normal-case-path and simplify-path on both paths is the best thing (before then using equal? on the result). But it may make sense to look around and see if there are other places where paths are compared and do what they're doing.

squarebat commented 2 years ago

That makes sense. I falsely assumed that all preferences were being handled by the framework.
Removing opened files might be a bit tricky since there are several ways in which files are being opened and all seem to have different underlying methods. I figured that it could be best dealt with by checking if a file is already opened in reopen-closed-tab. I checked out what the browser does and surprisingly it (mozilla and brave) will open the url even if it is already opened in another. We could do that too.
There is a size-down method in framework/private/handler.rkt for that. I thought about installing it as a public method but couldn't achieve it. But if the preference doesn't stay in the framework, it may be good to leave framework out entirely.
Thanks! I forgot to ask what start and end were being used for and ignored them for the time being.
The method recently-opened-files-same-enough-path? in handler does exactly that. I'll do the same.

rfindler commented 2 years ago

I see what you mean about removing items from the list (because they were opened elsewhere). I think we could achieve that goal if we wanted to (I can dig around to find the single point of control -- I am sure there is one or we could make one), but if you think it would be clearer to the users to not do that, I am on board.

Robby

squarebat commented 2 years ago

I think it would be more straightforward to just reopen the first file, even if it is already open. But rather than reopening the same file again, we can change to the tab where the file is open. Another issue with removing from the list is that there isn't really an easy way to remove files that don't exist anymore. So we would still have to loop through the list while reopening closed tabs until we find a suitable entry. That being said, digging around to find the single point sounds fun, I'll see if I find anything.

rfindler commented 2 years ago

That's a great point about just moving to the active tab. Indeed, unless we go to some lengths, just opening the file will send them to the tab where it is already open! Great!

squarebat commented 2 years ago

unless we go to some lengths

Indeed, the only way I was able to open the same file twice was by playing around with reopen-closed-tabs In the latest commit, I have added the preference to drracket/private/main. As discussed, if a closed file has already been opened, \-shift-t will redirect to active tab. I have also added code to remove duplicates from list and introduced an upper limit - recently-closed-tabs-max-count.

One problem though. I have retrieved the insertion positions from get-definitions-text, but the values will be lost since reopen-closed-tabs calls (open-in-new-tab filename) which ultimately calls create-new-tab where a new text object is created. Updating that new text object with the saved positions is fairly straightforward, but it'll require changing the create-new-tab method and adding default value parameters to it. Shall I go ahead with that?

rfindler commented 2 years ago

I think adding a keyword, optional argument that specifies the position to create-new-tab and open-in-new-tab is great. (The docs would need to be updated too, tho.)

I think that 'drracket:recently-closed-tabs-max-count can default to a larger number. The 50 in the other one is because they get put into a menu somewhere. But these won't be.

Because the definition of truncate-list is in a class, it is going to be a private field (i.e. per-object storage), so probably best to change it to define/private so that it will be a method (no extra storage in the object it is just once in the class).

Probably good to remove the shortcut for the other item -- or is that not in DrR? I'm forgetting where it is.

And there eventually needs to be an update to HISTORY.txt and an update to the documentation because there is a new menu item.

squarebat commented 2 years ago

Updated the args. They do make more sense as kwargs
Should History.txt be updated to reflect the addition of the keyword args?

squarebat commented 2 years ago

Are there any updates on this?

rfindler commented 2 years ago

Sorry for the long delay here @squarebat . I think that it is basically ready to go. I can address the two comments I put in and merge it, if you want?

squarebat commented 2 years ago

Sure!

On Fri, 1 Apr 2022, 20:16 Robby Findler, @.***> wrote:

Sorry for the long delay here @squarebat https://github.com/squarebat . I think that it is basically ready to go. I can address the two comments I put in and merge it, if you want?

— Reply to this email directly, view it on GitHub https://github.com/racket/drracket/pull/543#issuecomment-1085990716, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOCT4DMCPM7DYL634IKFIPTVC4D3PANCNFSM5MD5PBSQ . You are receiving this because you were mentioned.Message ID: @.***>

racket / drracket

New file menu item - Reopen Closed Tab #543