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

Notes are not saved after abnormal termination #8951

Closed
ZenulAbidin opened this issue Mar 14, 2024 · 7 comments
Closed

Notes are not saved after abnormal termination #8951

ZenulAbidin opened this issue Mar 14, 2024 · 7 comments

Comments

@ZenulAbidin
Copy link
Contributor

ZenulAbidin commented Mar 14, 2024

Version: Electrum 4.5.0, from Python tarball
OS: Ubuntu 22.04 LTS, with KDE Plasma 5.24.7 and Qt 5.15.3

Description:

ElectrumWindow.clean_up is not called when Electrum quits abnormally. For example, during Ctrl-C, system shut down or sign out. This means that wallet.db.put('notes_text', ... ) is never called and the note is lost. The notes are not written anywhere else in the program.

Code:

def closeEvent(self, event):
# note that closeEvent is NOT called if the user quits with Ctrl-C
self.clean_up()
event.accept()
def clean_up(self):
if self._cleaned_up:
return
self._cleaned_up = True
if self.thread:
self.thread.stop()
self.thread = None
with self._coroutines_scheduled_lock:
coro_keys = list(self._coroutines_scheduled.keys())
for fut in coro_keys:
fut.cancel()
self.unregister_callbacks()
self.config.GUI_QT_WINDOW_IS_MAXIMIZED = self.isMaximized()
self.wallet.db.put('notes_text', self.notes_tab.toPlainText())

Resolution:

The note should be autosaved whenever it is updated. This entails listening for when the Qt element is modified and then writing it out to the wallet file in a callback. I can help with that if necessary.

@ecdsa
Copy link
Member

ecdsa commented Mar 14, 2024

Currently it would be too expensive to save the wallet file on each keystroke.
We can do that once we have partial writes for encrypted files.

We have recently merged support for partial writes, but only for non-encrypted files, see #8493
There is also a draft branch that supports partial writes with encryption, see https://github.com/spesmilo/electrum/commits/jsonpatch
Basically, what is missing is a proper header, see #5999

@SomberNight
Copy link
Member

We could also just add a "Save" button though.

@ZenulAbidin
Copy link
Contributor Author

ZenulAbidin commented Mar 14, 2024

We could also just add a "Save" button though.

I came up with a workaround in the Console that emulates save/load functionality.

def save_notes():
    wallet.db.put('notes_text', window.notes_tab.toPlainText())

def load_notes():
    window.notes_tab.setPlainText(wallet.db.get('notes_text'))

Short-term, it works fine. Same for a save button in the GUI. Long-term though, I'm not sure if a manual save button is viable from a UX perspective over automatically saving the notes content.

@SomberNight
Copy link
Member

Currently it would be too expensive to save the wallet file on each keystroke.
We can do that once we have partial writes for encrypted files.

Partial writes of the wallet file would still not be sufficient as we are storing the notes as a single string. And the notes can become quite long, potentially hundreds of kilobytes. So what would be desired is partial updates of the notes itself (in addition to the wallet file). However, I don't think it's worth coming up with some complex design re how to store the text and update it cheaply, i.e. reimplementing more and more parts of a text editor.

Instead, again, we could just have a "save" button.

def load_notes():

I think a "load" button would not be useful. Whatever is displayed in the qt textedit should be the up to date string, and when the user clicks "save", we can persist it to the file (and also on graceful shutdown, as done already).

@ecdsa
Copy link
Member

ecdsa commented Mar 14, 2024

I don't want a new button for that. Remember, we are dealing with abnormal termination here.
Instead of saving on each keystroke, we could detect when the last keystroke is more than 10 seconds ago.

@ZenulAbidin
Copy link
Contributor Author

def load_notes():

I think a "load" button would not be useful. Whatever is displayed in the qt textedit should be the up to date string, and when the user clicks "save", we can persist it to the file (and also on graceful shutdown, as done already).

Yes, I agree. Considering that Electrum already loads the note, if any, at startup, it doesn't really have any value. I just wrote it for completeness.

@SomberNight
Copy link
Member

I don't want a new button for that

It is quite normal for text editors to have a save button. It is expected. (name one text editor that does not have one :P)
The button could go into the toolbar of the tab.

It is also more intuitive than a "delayed" automatic save, I think. We could even have both.
Or if you really do not want a save button, we could have just the delayed save, yes.

@ecdsa ecdsa closed this as completed in 17599de Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants