Crash in Windows 10 when drag and drop #559
Comments
|
Before I dig into it and chase my own tail trying to figure out how to reproduce it, can you offer any more information? Does it work with an empty folder? |
|
In "plan" mode, I have 4 folders (chapters) including 4 scenes. I created a new folder and wanted to move all four chapter in that new folder that I wanted to name Book 1, (inspired by the trilogy preset). I selected first the 4 folders at once and dragged them in the new folder (5th in the list) and it crashed. I tried the same but on folder (chapter) at a time... and after the 2nd, it crashed again. |
|
I have noticed a crash once in the past when I was organising way back when, but I wasn't particularly interesting in figuring out the cause back then. Anyway, I have tried moving around some folders in a test project just now and I can't seem to reproduce it though, even when following your instructions. (I assume you mean the outline screen when you talk about 'plan' mode). Two more questions that come to mind:
My guess is that you might be noticing some sort of race condition / inconsistency bug that is caused by the auto-save feature saving while the UI is trying to move files about when other parts of the code aren't expecting those bits to change. I've noticed things like those pop up once or twice in the past. |
|
I'm sorry to be so unprecise. I try to be clearer in the future.
I disabled revisions and now the drag and drop is working just fine. Thanks a lot! I believe that I can reactivate revision now. |
|
I am currently digging into the saving and outline drag-n-drop code, and I am not quite sure how those two interact as of yet. I did manage to cause a crash while messing around so far but it does not seem to involve saving functionality. Can you run Manuskript from a command prompt and reproduce the bug (preferably with a sample project) to see exactly what kind of messages and traceback it produces, and then share it with us? I'll experiment a bit more to see what else I can do to make it crash, but I want to fully understand the problem before I start to try and patch the one thing I have observed myself. |
Describing all the rabbitholes that I and kakaroto have gone through while debugging this one until dawn can frankly not do enough justice to the crazy amount of rubberducking that went on while trying to fix this. This bug would be triggered whenever you had a document open in the editor and then moved an ancestor object downwards (visually) in the tree. Or when you simply deleted the ancestor. Depending on the exact method that caused the opened item to be removed from the internal model, the exact nature of the bug would vary, which means this commit fixes a few different bits of code that lead to what appears to be the same bug. In order of appearance, the bugs that ruined our sleep were: 1) The editor widget was trying to handle the removed item at too late a stage. 2) The editor widget tried to fix its view after a move by searching for the new item with the same ID, but in the case of moving an object down it came across its own old item, ruining the attempt. 3) The editor widget did not properly account for the hierarchical nature of the model. Upon fixing these the next day, it was revealed that: 4) The outlineItem.updateWordCount(emit=False) flag is broken. This function would call setData() in several spots which would still cause emits to bubble through the system despite emit=False, and we simply got lucky that it stopped enough of them until now. This last one was caused by a small mistake in the fixes for the first three bugs, but it has led to a couple of extra changes to make any future bug hunts slightly less arduous and frustrating: a) When calling item.removeChild(c), it now resets the associated parent and model to mirror item.insertChild(c). This has also led to an extra check in model.parent() to check for its validity. b) The outlineItem.updateWordCount(emit=) flag has been removed entirely and it now emits away with reckless abandon. I have been unable to reproduce the crashes the code warned about, so I consider this a code quality fix to prevent mysterious future issues where things sometimes do not properly update right. Worthy of note is that the original code clearly showed the intention to close tabs for items that were removed. Reworking the editor to support closing a tab is unfortunately way out of scope, so this intention was left in and the new fix was structured to make it trivial to implement such a change when the time comes. An existing FIXME regarding unrelated buggy editor behaviour was left in, too. Many thanks to Kakaroto for burning the midnight oil with me to get to the bottom of this. (I learned a lot that night!) Issues olivierkes#479 and olivierkes#559 are fixed by this commit. And maybe some others, too.
Describing all the rabbitholes that I and kakaroto have gone through while debugging this one until dawn can frankly not do enough justice to the crazy amount of rubberducking that went on while trying to fix this. This bug would be triggered whenever you had a document open in the editor and then moved an ancestor object downwards (visually) in the tree. Or when you simply deleted the ancestor. Depending on the exact method that caused the opened item to be removed from the internal model, the exact nature of the bug would vary, which means this commit fixes a few different bits of code that lead to what appears to be the same bug. In order of appearance, the bugs that ruined our sleep were: 1) The editor widget was trying to handle the removed item at too late a stage. 2) The editor widget tried to fix its view after a move by searching for the new item with the same ID, but in the case of moving an object down it came across its own old item, ruining the attempt. 3) The editor widget did not properly account for the hierarchical nature of the model. Upon fixing these the next day, it was revealed that: 4) The outlineItem.updateWordCount(emit=False) flag is broken. This function would call setData() in several spots which would still cause emits to bubble through the system despite emit=False, and we simply got lucky that it stopped enough of them until now. This last one was caused by a small mistake in the fixes for the first three bugs, but it has led to a couple of extra changes to make any future bug hunts slightly less arduous and frustrating: a) When calling item.removeChild(c), it now resets the associated parent and model to mirror item.insertChild(c). This has also led to an extra check in model.parent() to check for its validity. b) The outlineItem.updateWordCount(emit=) flag has been removed entirely and it now emits away with reckless abandon. I have been unable to reproduce the crashes the code warned about, so I consider this a code quality fix to prevent mysterious future issues where things sometimes do not properly update right. Worthy of note is that the original code clearly showed the intention to close tabs for items that were removed. Reworking the editor to support closing a tab is unfortunately way out of scope, so this intention was left in and the new fix was structured to make it trivial to implement such a change when the time comes. An existing FIXME regarding unrelated buggy editor behaviour was left in, too. Many thanks to Kakaroto for burning the midnight oil with me to get to the bottom of this. (I learned a lot that night!) Issues olivierkes#479 and olivierkes#559 are fixed by this commit. And maybe some others, too. Changes according to code review Removed some pointless checks. Also cleaned up some variable names, comments and a docstring. It also adds a bugfix for non-updating tab titles in the editor.
Describing all the rabbitholes that I and kakaroto have gone through while debugging this one until dawn can frankly not do enough justice to the crazy amount of rubberducking that went on while trying to fix this. This bug would be triggered whenever you had a document open in the editor and then moved an ancestor object downwards (visually) in the tree. Or when you simply deleted the ancestor. Depending on the exact method that caused the opened item to be removed from the internal model, the exact nature of the bug would vary, which means this commit fixes a few different bits of code that lead to what appears to be the same bug. In order of appearance, the bugs that ruined our sleep were: 1) The editor widget was trying to handle the removed item at too late a stage. 2) The editor widget tried to fix its view after a move by searching for the new item with the same ID, but in the case of moving an object down it came across its own old item, ruining the attempt. 3) The editor widget did not properly account for the hierarchical nature of the model. Upon fixing these the next day, it was revealed that: 4) The outlineItem.updateWordCount(emit=False) flag is broken. This function would call setData() in several spots which would still cause emits to bubble through the system despite emit=False, and we simply got lucky that it stopped enough of them until now. This last one was caused by a small mistake in the fixes for the first three bugs, but it has led to a couple of extra changes to make any future bug hunts slightly less arduous and frustrating: a) When calling item.removeChild(c), it now resets the associated parent and model to mirror item.insertChild(c). This has also led to an extra check in model.parent() to check for its validity. b) The outlineItem.updateWordCount(emit=) flag has been removed entirely and it now emits away with reckless abandon. I have been unable to reproduce the crashes the code warned about, so I consider this a code quality fix to prevent mysterious future issues where things sometimes do not properly update right. Worthy of note is that the original code clearly showed the intention to close tabs for items that were removed. Reworking the editor to support closing a tab is unfortunately way out of scope, so this intention was left in and the new fix was structured to make it trivial to implement such a change when the time comes. An existing FIXME regarding unrelated buggy editor behaviour was left in, too. Many thanks to Kakaroto for burning the midnight oil with me to get to the bottom of this. (I learned a lot that night!) Issues olivierkes#479 and olivierkes#559 are fixed by this commit. And maybe some others, too. Changes according to code review It also adds a bugfix for non-updating tab titles in the editor. Fix for issue olivierkes#516 + extra safety checks. A modified safety check has been added back in for the signals where these could protect against a buggy situation. Technically speaking, olivierkes#516 is out of scope... but it is a trivial fix, so why not? :-)
|
Can someone provide an exact list of steps that reproduce this crash? Following is the testing I performed. Test 1 - Drag and Drop Chapter into other Chapter
OBSERVATIONS:
Test 2 - Drag and Drop Chapter beside other Chapters
OBSERVATIONS:
SUMMARYSo far I am unable to reproduce the crash. Note that I do have Edit -> Settings -> Revisions enabled. EDIT: I have also tried changing step 2 to be "Select Outline pane" and I am still unable to reproduce the crash. |
|
It only crashes IF a text being moved (a child of the chapter) is open in the editor pane AND you do the move in the Outline pane, AND you move the chapter down not up in the order (For example, moving chapter 2 between chapters 3 and 4 would crash, it wouldn't crash if you move chapter 3 between chapters 1 and 2) |
|
Thanks @kakaroto for the tip. EDIT: My bad - was testing with PR 571 applied. Following are the steps I used to test. Test 3 - Drag and Drop Chapter with Open Text beside other Chapters
With Manuskript 0.9.0, a crash occurs at completion of step 7.
|
|
Ha! I just tested and confirmed the issue, came to say I don't know why you don't get the crash, and saw the edit! |
|
'Just finished testing with Windows 10. Both Windows 10 and Kubuntu 16.04 crash using the above Test 3 steps with Manuskript 0.9.0. When PR 571 is applied, both Windows 10 and Kubuntu 16.04 do not crash. |
|
@gedakc After having all those emails while out for dinner giving me a serious pit-of-my-stomach feeling about screwing up the fix, that is literally the best comment you could have posted tonight. 😄 |
|
@worstje sorry about that. My bad not using the master branch to confirm the original crash report. |
Describing all the rabbitholes that I and kakaroto have gone through while debugging this one until dawn can frankly not do enough justice to the crazy amount of rubberducking that went on while trying to fix this. This bug would be triggered whenever you had a document open in the editor and then moved an ancestor object downwards (visually) in the tree. Or when you simply deleted the ancestor. Depending on the exact method that caused the opened item to be removed from the internal model, the exact nature of the bug would vary, which means this commit fixes a few different bits of code that lead to what appears to be the same bug. In order of appearance, the bugs that ruined our sleep were: 1) The editor widget was trying to handle the removed item at too late a stage. 2) The editor widget tried to fix its view after a move by searching for the new item with the same ID, but in the case of moving an object down it came across its own old item, ruining the attempt. 3) The editor widget did not properly account for the hierarchical nature of the model. Upon fixing these the next day, it was revealed that: 4) The outlineItem.updateWordCount(emit=False) flag is broken. This function would call setData() in several spots which would still cause emits to bubble through the system despite emit=False, and we simply got lucky that it stopped enough of them until now. This last one was caused by a small mistake in the fixes for the first three bugs, but it has led to a couple of extra changes to make any future bug hunts slightly less arduous and frustrating: a) When calling item.removeChild(c), it now resets the associated parent and model to mirror item.insertChild(c). This has also led to an extra check in model.parent() to check for its validity. b) The outlineItem.updateWordCount(emit=) flag has been removed entirely and it now emits away with reckless abandon. I have been unable to reproduce the crashes the code warned about, so I consider this a code quality fix to prevent mysterious future issues where things sometimes do not properly update right. Worthy of note is that the original code clearly showed the intention to close tabs for items that were removed. Reworking the editor to support closing a tab is unfortunately way out of scope, so this intention was left in and the new fix was structured to make it trivial to implement such a change when the time comes. An existing FIXME regarding unrelated buggy editor behaviour was left in, too. Many thanks to Kakaroto for burning the midnight oil with me to get to the bottom of this. (I learned a lot that night!) Issues olivierkes#479, olivierkes#516 and olivierkes#559 are fixed by this commit. And maybe some others, too.
Describing all the rabbitholes that I and kakaroto have gone through while debugging this one until dawn can frankly not do enough justice to the crazy amount of rubberducking that went on while trying to fix this. This bug would be triggered whenever you had a document open in the editor and then moved an ancestor object downwards (visually) in the tree. Or when you simply deleted the ancestor. Depending on the exact method that caused the opened item to be removed from the internal model, the exact nature of the bug would vary, which means this commit fixes a few different bits of code that lead to what appears to be the same bug. In order of appearance, the bugs that ruined our sleep were: 1) The editor widget was trying to handle the removed item at too late a stage. 2) The editor widget tried to fix its view after a move by searching for the new item with the same ID, but in the case of moving an object down it came across its own old item, ruining the attempt. 3) The editor widget did not properly account for the hierarchical nature of the model. Upon fixing these the next day, it was revealed that: 4) The outlineItem.updateWordCount(emit=False) flag is broken. This function would call setData() in several spots which would still cause emits to bubble through the system despite emit=False, and we simply got lucky that it stopped enough of them until now. This last one was caused by a small mistake in the fixes for the first three bugs, but it has led to a couple of extra changes to make any future bug hunts slightly less arduous and frustrating: a) When calling item.removeChild(c), it now resets the associated parent and model to mirror item.insertChild(c). This has also led to an extra check in model.parent() to check for its validity. b) The outlineItem.updateWordCount(emit=) flag has been removed entirely and it now emits away with reckless abandon. I have been unable to reproduce the crashes the code warned about, so I consider this a code quality fix to prevent mysterious future issues where things sometimes do not properly update right. Worthy of note is that the original code clearly showed the intention to close tabs for items that were removed. Reworking the editor to support closing a tab is unfortunately way out of scope, so this intention was left in and the new fix was structured to make it trivial to implement such a change when the time comes. An existing FIXME regarding unrelated buggy editor behaviour was left in, too. Many thanks to Kakaroto for burning the midnight oil with me to get to the bottom of this. (I learned a lot that night!) Issues #479, #516 and #559 are fixed by this commit. And maybe some others, too.
|
This issue has been resolved in the develop branch and will be included in the next release of Manuskript. |
In Windows 10, latest version of Manuskript. I try dragging and dropping one or many folder (including scenes) in another folder and it crashs. I tried with one folder... and many folders and still it crash. In fact. When I drag and drop individual folder. The first transfer don't crash, but the second crash.
The text was updated successfully, but these errors were encountered: