Review Request 122382: [klipper] Sync history to disk after each change

Mon Feb 16 08:13:16 UTC 2015

> On Feb. 3, 2015, 8:36 a.m., Martin Gräßlin wrote:
> > David E. just pointed out that this could become quite heavy for the system as the history size can be large (up to 2048 items).
> 
> Martin Gräßlin wrote:
>     Unfortunately I couldn't find out why we support up to 2048 items. Commit message is just:
>     
>     commit da8394ce42a24726392265436c3808f1ac9389aa
>     Author: Esben Mose Hansen <kde at mosehansen.dk>
>     Date:   Fri Nov 19 22:28:55 2004 +0000
>     
>         Introduced support for large clipboard histories up to 2048 items.
>         
>         svn path=/trunk/kdebase/klipper/; revision=364353
> 
> Martin Klapetek wrote:
>     I think 2048 is insane. Can we make it like 32 by default and have it configurable with big fat warning when you choose more than say 100? 
>     
>     Btw. does klipper store things encrypted or something? There's also a security concern, especially if your klipper contains passwords, that saving those to disk unecrypted after each copy is insecure (all you need is a watcher on the history file).
> 
> Martin Gräßlin wrote:
>     > I think 2048 is insane. Can we make it like 32 by default and have it configurable with big fat warning when you choose more than say 100? 
>     
>     The default is 7. Adding a warning is certainly possible.
>     
>     > Btw. does klipper store things encrypted or something? There's also a security concern, especially if your klipper contains passwords, that saving those to disk unecrypted after each copy is insecure (all you need is a watcher on the history file).
>     
>     If you are able to watch the file you are also able to connect to the X11 Display and just do a passive keyboard grab. So caring about that probably doesn't matter (on Wayland this might get more important - maybe we can skip passwords). But setting the file to 600 is certainly a good idea.
> 
> Martin Gräßlin wrote:
>     > But setting the file to 600 is certainly a good idea.
>     
>     this seems already to be the case (though I don't find the code for it)
> 
> Filip Wieladek wrote:
>     FYI: I happened to see cross this. I only use Klipper at 2048. With such a size of the data, it means that I usually have my most frequently used items always available.
> 
> Martin Gräßlin wrote:
>     I'm still unsure what to do about this one. If we go for syncing to disk we probably break workflows like Filip's. Maybe we need to add an additional option to automatically sync and show a warning if the history size gets larger than e.g. 20?
> 
> Filip Wieladek wrote:
>     I don't know how much work it would be, but would it be possible to separate the klipper history engine into a seperate daemon process and communicate with it over the DBUS? In this case the daemon would be much less likely to crash (due to plasma) and would not have to rely on frequent disk access.
>     
>     If that is not possible, or too much work, why dont we simply take a snapshot, convert the history into a QList (that should be fast) and queue up for a save? The save process could then be independent and save a bit later (it could also debounce the save's).
>     
>     However, there might be one more problem. If the plasmashell crashing is really an issue, what happens if plasmashell crashes during a save operation? Now it has the potential to corrupt the data losing all the history instead of the latest entries. That means, that his approach would need to use at least 2 files, to write in a rotation, so that there is at least one good file to read from.

> I don't know how much work it would be, but would it be possible to separate the klipper history engine into a seperate daemon process and communicate with it over the DBUS?

I think it's too much work, especially to keep everything race free (changing clipboard could happen from multiple places in async way) and up to date.

> If that is not possible, or too much work, why dont we simply take a snapshot, convert the history into a QList (that should be fast) and queue up for a save? The save process could then be independent and save a bit later (it could also debounce the save's).

A throttling could also be implemented directly without the need to add a separate process.

> Now it has the potential to corrupt the data losing all the history instead of the latest entries. That means, that his approach would need to use at least 2 files, to write in a rotation, so that there is at least one good file to read from.

That should not be an issue as QSaveFile is used.

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122382/#review75264
-----------------------------------------------------------

On Feb. 2, 2015, 4:12 p.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122382/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2015, 4:12 p.m.)
> 
> 
> Review request for Plasma and Eike Hein.
> 
> 
> Bugs: 343333
>     https://bugs.kde.org/show_bug.cgi?id=343333
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> By invoking saveHistory after each change we ensure that the clipboard
> doesn't lose data in case klipper (or in dataengine mode plasmashell)
> crashes.
> 
> To not cause stalls, the saving is performed in a thread using
> QtConcurrentRun. As klipper itself is not thread save a Mutex is
> used to lock changes in the HistoryModel.
> 
> BUG: 343333
> FIXED-IN: 5.3.0
> 
> 
> Diffs
> -----
> 
>   klipper/klipper.cpp d49c165759f8171931167687c3b36b3a9d7dee07 
>   klipper/CMakeLists.txt a08f062480b15f32f049e2d0d0e311dbe2964c02 
>   klipper/historymodel.h 78f955f0ec4b8f27dbca0573b68691be6a30e3be 
>   klipper/historymodel.cpp 51860f6c3aca1022a2b721c27c859fc721915353 
> 
> Diff: https://git.reviewboard.kde.org/r/122382/diff/
> 
> 
> Testing
> -------
> 
> looked at ~/.local/share/klipper/history2.lst in Okteta, changed clipboard and pressed F5 in Okteta. Repeated these steps multiple times.
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150216/ab728589/attachment.html>