spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.22k stars 3.02k forks source link

Notes are not saved after abnormal termination #8951

Closed ZenulAbidin closed 3 months ago

ZenulAbidin commented 3 months ago

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: https://github.com/spesmilo/electrum/blob/99f6dd5d5d63bdb311bd401835cb20423728f889/electrum/gui/qt/main_window.py#L2506-L2524

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 commented 3 months ago

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 commented 3 months ago

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

ZenulAbidin commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.