Discard history on project-file save/load #2400

ferdnyc · 2018-11-25T23:32:00Z

On both load and save of project file data, any "history" in the JSON will be discarded, replaced with an empty struct:

self._data["history"] = { "undo": [], "redo": [] }

Temporarily, while testing the feature, the total size of the data structure being discarded will first be computed and logged. (I added a total_size() function, taken from a python recipe — there's a link in the comment header — to classes/conversion.py, which maybe isn't the best spot but oh well.)

log.info("Discarding history of size {} bytes".format(total_size(self._data["history"])))
# Produces:
# project_data:INFO Discarding history of size 105246892 bytes

(That's @peanutbutterandcrackers' 12MB mega-exploded project file — turns out, that history turns into One Hundred Five megabytes in memory!)

In my testing, this works seamlessly. History operates normally when using OpenShot, and can be undone and redone as usual. Even after saving a project, the history is still there in that session. However, when you load a project, instead of the Undo history being populated, it's now always cleared, so history only persists through the session.

If you load old project files and resave them, the history will be removed. (Actually, that can happen anyway with autosave — I've accidentally lost the long history in more than a few of my test projects already, in testing this code out. Which is making it harder to actually test! But, in a good way. And I could always turn off autosave.)

Kick the tires, see what you think. Hopefully this gives OpenShot a kick in the ass, performance-wise, especially for people with project files that have ballooned in size. And it should also eliminate any history crashes caused by absolute paths to AppImage mounts.

Fixes #1923
Addresses (possibly) #1768

Believe it or not, in all the logging OpenShot does, never was the fact that a project file was being loaded or saved actually logged as such. Now it is, complete with the `file_path`.

On both load and save of project file data, any `"history"` in the JSON will be discarded, replaced with an empty struct: `{ "undo": [], "redo": [] }` Temporarily, while testing the feature, the total size of the data structure being discarded will first be computed and logged. ``` project_data:INFO Discarding history of size 105246892 bytes ```

ferdnyc · 2018-11-25T23:36:33Z

P.S> 56fa87a adds logging at the start of project-file save and load operations — previously it only logged at the completion of those operations, which is no help from a debugging standpoint. You want to know if anything untoward happens during the save/load operation.

DylanC · 2018-11-26T15:50:53Z

@ferdnyc - I'm supposed to be on a break from this project but I'm very interested and excited to test this change. This is the best PR ever for the project in recent times and could solve a whole lot of bad experiences related to performance issues across the board.

DylanC · 2018-11-26T20:06:01Z

@ferdnyc - I had a brief test of this and it didn't appear to be saving any undo history during my session. I tested both an old project and created a new one.

Should I have deleted the Openshot preferences folder before testing this?

ferdnyc · 2018-11-26T20:18:06Z

@ferdnyc - I had a brief test of this and it didn't appear to be saving any undo history during my session. I tested both an old project and created a new one.

Meaning, undo didn't work within your session? Oh, it's not supposed to break that! I thought I'd tested that from every angle but I may have missed something.

Should I have deleted the Openshot preferences folder before testing this?

No, there are no preferences changes or changes to the default project, only save/load of project file data are supposed to be affected. But maybe I missed something in how new-project setup is handled. I'll test through project creation, especially, more thoroughly and carefully when I can... later tonight or tomorrow, hopefully.

DylanC · 2018-11-26T20:26:37Z

@ferdnyc - Nevermind. I think I was a bit confused on what I was looking for here. I had the project file open in an editor and for some reason expected the undo json to be populated with data.

Undo and Redo did work in my session. 😆

ferdnyc · 2018-11-26T20:49:36Z

I had the project file open in an editor and for some reason expected the undo json to be populated with data.

Yeah, that's specifically what this PR does away with, because of all the problems (performance, path issues, outright crashing) it's caused to be storing history.

I still feel it's a nice feature to provide history persistence across sessions. But only if it can be done safely, reusably, and without destroying OpenShot's performance, which the current implementation falls short of.

ferdnyc · 2018-11-26T21:16:05Z

openshot-qt/src/classes/updates.py

Lines 72 to 84 in e8633bf

    
           # Build the dictionary to be serialized 
        
           if only_value: 
        
               data_dict = copy.deepcopy(self.values) 
        
           else: 
        
               data_dict = {"type": self.type, 
        
                            "key": self.key, 
        
                            "value": copy.deepcopy(self.values), 
        
                            "partial": self.partial_update, 
        
                            "old_values": copy.deepcopy(self.old_values)} 
        
           # Always remove 'history' key (if found) 
        
           if "history" in data_dict: 
        
               data_dict.pop("history")

My suspicion, BTW, is that the bug that blows out the history lives right here in the code. You can see where it tests if "history" in data_dict right at the end, intending to drop the history — but unless we're in the only_value case and data_dict is a deepcopy() of self.values, it'll never contain "history". We can SEE all of the keys it contains — they're explicitly set in the lines immediately previous. "history" isn't one of them.

I think the test should be something different, at least for the non only_value case. (It may be fine, and need to be kept, for the only_value case.) I'm just not sure what the correct fix is, because I'm not clear on the intent of that code. (The comment is more narrative than explanatory.) But most likely it's either trying to examine the key, or the value. So, maybe...

if "history" in data_dict["key"]:
  do_something() # though I'm honestly not sure what
# or
if "history" in data_dict["value"]:
  data_dict["value"].pop("history")

(I know that updates with "key": [ "history" ] do exist, and they're the ones that blow out the project files. So, the first option may very well be correct. But I still don't know what do_something() should be in that case. Quite possibly, it should be to just return from the function completely at that point, bailing on doing the update processing since history updates shouldn't be tracked as UpdateActions — that's where we get into trouble with recursive history stacks.)

ferdnyc · 2018-11-26T21:36:38Z

Yeah, that's specifically what this PR does away with, because of all the problems (performance, path issues, outright crashing) it's caused to be storing history.

I should also note that I'd prefer to have @jonoomph OK this change before it merges, since history persistence across sessions has been explicitly held up as a desirable feature (see e.g. 86f1d48) and it doesn't feel like it should be my call to do away with that, whatever problems it's causing.

(The recursive history entries is only one of those problems — the other major one being outright crashes, caused by expired AppImage transition paths in the history — but the recursion was thought fixed in #2084, the change that introduced the code I highlighted in the previous comment.)

peanutbutterandcrackers · 2018-11-27T03:30:31Z

Since

This PR is going to fix a lot of performance issues
Not even GIMP (2.10.8, at least), nor LibreOffice (6.0.6.2, at least), neither InkScape (0.92, at least) saves history across sessions
OpenShot is essentially non-destructive, and users can manually change every parameter they've set
This PR has passed all checks
This PR has been tested

I'd be happy to take responsibility for the merge, if you two are not against it.

ferdnyc · 2018-11-27T06:53:04Z

I'd prefer to have @jonoomph OK this change before it merges

DylanC · 2018-11-27T10:36:16Z

@ferdnyc - I'm happy to leave it up to @jonoomph. Its more of a fundamental change to the project.

DylanC · 2018-12-18T10:05:54Z

@jonoomph - Can you review this change?

ferdnyc · 2018-12-18T17:03:03Z

@DylanC : Ah, I remember optimism.

My favorite is when he parachutes in to merge his own PRs and still ignores all the rest of them (e.g.: #2450)

peanutbutterandcrackers · 2018-12-18T18:24:26Z

@ferdnyc - We have been notified about this. The plan is to clean up the newer commits to make them translate-able, I guess. Once that is done, plans are to go through each of the PRs. That is why the optimism. Hopefully, this (and other PRs) will be merged really soon.

ferdnyc · 2018-12-18T20:56:31Z

This PR contains no translatable strings, its only output is to the log.

peanutbutterandcrackers · 2018-12-19T04:36:22Z

Not this PR, sir. The others that have been merged. I guess he's just trying to polish up what is in the main tree already and once that is done (which I hope it is) he'll be in the currently open PRs. Hope that clears up the confusion.

jonoomph · 2018-12-20T22:19:20Z

Heyyo @ferdnyc ! Looking at this now!

jonoomph · 2018-12-20T22:21:25Z

Yeah, originally I thought this would be a great feature, to persist history across sessions. However, as noted above, this has some ugly repercussions... such as bloated file sizes and incorrect paths. One issue I "thought" was resolved (but might not be), is the storing of nested histories. For example, a project has history [], and then someone would save or load a project, which would add the entire project, plus the entire history [] as a nested structure... thus really, really, really ballooning the size. That might still be happening, not sure.

jonoomph · 2018-12-20T22:25:41Z

Merging this into a test branch, so I can see if it passes all the different OS build servers

jonoomph · 2018-12-20T22:27:34Z

Also, I'm not sure if the risk of total_size() method is really needed. I might remove that logging before merging this into develop, FYI... unless there is a really critical reason to keep it. =)

* Discard history on project-file save/load (#2400) * updates.py: add newline at end of file * conversion: total_size() for data structs * project_data: Log load and save operations Believe it or not, in all the logging OpenShot does, never was the fact that a project file was being loaded or saved actually logged as such. Now it is, complete with the `file_path`. * project_data: Discard history On both load and save of project file data, any `"history"` in the JSON will be discarded, replaced with an empty struct: `{ "undo": [], "redo": [] }` Temporarily, while testing the feature, the total size of the data structure being discarded will first be computed and logged. ``` project_data:INFO Discarding history of size 105246892 bytes ``` * Removing logging of history size, since it does not serve much of a purpose. Soon we need to revisit this and try and improve the persistance of history (within reason to a few dozen changes perhaps), and ensure we don't allow nested histories (since history is an attribute of a project object).

jonoomph · 2018-12-21T04:43:06Z

Merged into develop! Thanks @ferdnyc !

ferdnyc added 4 commits November 25, 2018 18:03

updates.py: add newline at end of file

62048f5

conversion: total_size() for data structs

4a1058c

project_data: Log load and save operations

56fa87a

Believe it or not, in all the logging OpenShot does, never was the fact that a project file was being loaded or saved actually logged as such. Now it is, complete with the `file_path`.

ferdnyc changed the title ~~Discard history on project-file save/reload~~ Discard history on project-file save/load Nov 25, 2018

This was referenced Nov 26, 2018

[WIP] project_data: Store thumbnail paths aliased #2312

Closed

Help! Christmas Vid "'NoneType object is not subscriptable" error #1151

Closed

Merge branch 'develop' into history-saving

b6b6a72

ferdnyc changed the title ~~Discard history on project-file save/load~~ WIP: Discard history on project-file save/load Nov 26, 2018

ferdnyc changed the title ~~WIP: Discard history on project-file save/load~~ Discard history on project-file save/load Nov 26, 2018

ferdnyc mentioned this pull request Nov 27, 2018

OpenShot Video Projects - Reopening from Google Drive, Multiple Sessions? #2374

Closed

ferdnyc added 2 commits December 2, 2018 09:14

Merge branch 'develop' into history-saving

49dd3d6

Merge branch 'develop' into history-saving

fcfefc8

ferdnyc mentioned this pull request Dec 13, 2018

Time on timeline incorrect #1768

Closed

Merge branch 'develop' into history-saving

2af827e

This was referenced Dec 15, 2018

SVG files not rendered properly #1081

Closed

'NoneType' object has no attribute 'get' #1051

Closed

Merge branch 'develop' into history-saving

f774cf6

jonoomph changed the base branch from develop to smaller-history-test December 20, 2018 22:24

jonoomph merged commit 3483e19 into OpenShot:smaller-history-test Dec 20, 2018

jonoomph mentioned this pull request Dec 21, 2018

Discard history on project-file save/load #2468

Merged

ferdnyc deleted the history-saving branch April 14, 2019 06:30

ferdnyc mentioned this pull request Dec 18, 2019

another "invalid load key" bug (urgent) #3126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discard history on project-file save/load #2400

Discard history on project-file save/load #2400

ferdnyc commented Nov 25, 2018 •

edited

Loading

ferdnyc commented Nov 25, 2018

DylanC commented Nov 26, 2018

DylanC commented Nov 26, 2018

ferdnyc commented Nov 26, 2018

DylanC commented Nov 26, 2018 •

edited

Loading

ferdnyc commented Nov 26, 2018

ferdnyc commented Nov 26, 2018

ferdnyc commented Nov 26, 2018

peanutbutterandcrackers commented Nov 27, 2018

ferdnyc commented Nov 27, 2018

DylanC commented Nov 27, 2018

DylanC commented Dec 18, 2018

ferdnyc commented Dec 18, 2018

peanutbutterandcrackers commented Dec 18, 2018 •

edited

Loading

ferdnyc commented Dec 18, 2018

peanutbutterandcrackers commented Dec 19, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 21, 2018

Discard history on project-file save/load #2400

Discard history on project-file save/load #2400

Conversation

ferdnyc commented Nov 25, 2018 • edited Loading

ferdnyc commented Nov 25, 2018

DylanC commented Nov 26, 2018

DylanC commented Nov 26, 2018

ferdnyc commented Nov 26, 2018

DylanC commented Nov 26, 2018 • edited Loading

ferdnyc commented Nov 26, 2018

ferdnyc commented Nov 26, 2018

ferdnyc commented Nov 26, 2018

peanutbutterandcrackers commented Nov 27, 2018

ferdnyc commented Nov 27, 2018

DylanC commented Nov 27, 2018

DylanC commented Dec 18, 2018

ferdnyc commented Dec 18, 2018

peanutbutterandcrackers commented Dec 18, 2018 • edited Loading

ferdnyc commented Dec 18, 2018

peanutbutterandcrackers commented Dec 19, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 20, 2018

jonoomph commented Dec 21, 2018

ferdnyc commented Nov 25, 2018 •

edited

Loading

DylanC commented Nov 26, 2018 •

edited

Loading

peanutbutterandcrackers commented Dec 18, 2018 •

edited

Loading