Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PDF reader: Undo support #2051

Open
dstillman opened this issue May 7, 2021 · 14 comments
Open

PDF reader: Undo support #2051

dstillman opened this issue May 7, 2021 · 14 comments
Assignees
Milestone

Comments

@dstillman
Copy link
Member

We probably want a more generalized undo solution that we can hook into for this, and that's a larger technical design question, but there are a few operations in the PDF reader where undoing would be particularly useful, such as creating and deleting annotations.

I think I have a branch somewhere where I've started playing with generalized undo/redo, so let's discuss before we do any work on this. (We could consider doing one-off implementations, but that might set up the expectation that undo/redo would work more generally, or with multiple history levels.)

@humanfactors
Copy link

I understand this is specific to the PDF reader, but I think a generalized undo/redo feature would be extremely useful. I frequently have accidentally modified a record, or erroneously drag'n'dropped a PDF to another record, but was unable to undo. I am not sure whether I should make a separate issue (or if there is much utility in doing so). However, I just wanted to flag this as an important possible feature :)

@mrtcode
Copy link
Member

mrtcode commented May 25, 2021

Even though annotation saving is debounced and there are some other asynchronous tasks before annotation is finally saved as Zotero item, I think we should still try to explore a general undo mechanism first.

I'm wondering if this would work for notes as well, because we also don't have a shared undo stack between note instances, and even for the same note if you select another note and then go back to the first one, it loses the undo stack.

@dstillman I would like to try out that branch.

@mrtcode mrtcode self-assigned this Sep 1, 2021
@dstillman
Copy link
Member Author

@mrtcode
Copy link
Member

mrtcode commented Apr 27, 2022

Should I try to implement a temporary and limited undo solution for the last action only, until we implement a more general undo/redo?

@dstillman
Copy link
Member Author

So I assume we'd clear the history when switching to another tab, which is easy, but I think we'd also want to clear when editing the item or creating/editing/deleting a note in the context pane. That seems a little harder, but if we can do that, I think we could do this — it's mostly a few actions within the reader that people have been asking for undo for.

@mrtcode
Copy link
Member

mrtcode commented Apr 28, 2022

But why do we want to clear the history?

@dstillman
Copy link
Member Author

Because otherwise you would perform some action elsewhere — e.g., editing an item field — but Undo would still be for the thing you did in the PDF reader rather than the last action you actually performed. If we can't undo the last action you actually performed, Undo should be disabled.

But maybe rather than clearing when switching tabs, which is a little weird, or watching for specific actions in the context pane, which is complicated, we could just watch for certain global Notifier events? Basically, any add/modify/delete that's not the last undo action would clear the undo state?

@dstillman
Copy link
Member Author

Well, that doesn't totally work, because sync and other things can trigger those. So maybe we need to mark notifier events as being background actions when they come from things like sync…

@dstillman
Copy link
Member Author

Unless you can think of a simpler option here, I'd hold off on this, and maybe we can work towards categorizing notifier events if we think that will be the best solution.

@stakats stakats added the RAMP label Jun 23, 2022
@stakats
Copy link
Member

stakats commented Jun 23, 2022

Generalized undo functionality is a deliverable for the RAMP project, but even partial implementation would be good to show progress here.

@dstillman dstillman added this to the RAMP milestone Jun 23, 2022
@dstillman dstillman removed the RAMP label Jun 23, 2022
@dstillman
Copy link
Member Author

@dstillman
Copy link
Member Author

@dstillman
Copy link
Member Author

@dstillman
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants