mirror of
https://github.com/olivierkes/manuskript.git
synced 2024-05-21 13:22:29 +12:00
Fix occasional crashes when (re)moving items
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 commit is contained in:
parent
385396c089
commit
12390a9aab
|
@ -141,6 +141,9 @@ class abstractItem():
|
|||
@return: the removed abstractItem
|
||||
"""
|
||||
r = self.childItems.pop(row)
|
||||
# Disassociate the child from its parent and the model.
|
||||
r._parent = None
|
||||
r.setModel(None)
|
||||
return r
|
||||
|
||||
def parent(self):
|
||||
|
|
|
@ -92,9 +92,15 @@ class abstractModel(QAbstractItemModel):
|
|||
"""
|
||||
return self.rootItem.findItemsContaining(text, columns, mainWindow(), caseSensitive)
|
||||
|
||||
def getItemByID(self, ID):
|
||||
def getItemByID(self, ID, ignore=None):
|
||||
"""Returns the item whose ID is `ID`, unless this item matches `ignore`."""
|
||||
|
||||
def search(item):
|
||||
if item.ID() == ID:
|
||||
if item == ignore:
|
||||
# The item we really want won't be found in the children of this
|
||||
# particular item anymore; stop searching this branch entirely.
|
||||
return None
|
||||
return item
|
||||
for c in item.children():
|
||||
r = search(c)
|
||||
|
@ -104,9 +110,12 @@ class abstractModel(QAbstractItemModel):
|
|||
item = search(self.rootItem)
|
||||
return item
|
||||
|
||||
def getIndexByID(self, ID, column=0):
|
||||
"Returns the index of item whose ID is `ID`. If none, returns QModelIndex()."
|
||||
item = self.getItemByID(ID)
|
||||
def getIndexByID(self, ID, column=0, ignore=None):
|
||||
"""Returns the index of item whose ID is `ID`. If none, returns QModelIndex().
|
||||
|
||||
If `ignore` is set, it will not return that item if found as valid match for the ID"""
|
||||
|
||||
item = self.getItemByID(ID, ignore=ignore)
|
||||
if not item:
|
||||
return QModelIndex()
|
||||
else:
|
||||
|
@ -119,7 +128,10 @@ class abstractModel(QAbstractItemModel):
|
|||
childItem = index.internalPointer()
|
||||
parentItem = childItem.parent()
|
||||
|
||||
if parentItem == self.rootItem:
|
||||
# Check whether the parent is the root, or is otherwise invalid.
|
||||
# That is to say: no parent or the parent lacks a parent.
|
||||
if (parentItem == self.rootItem) or \
|
||||
(parentItem is None) or (parentItem.parent() is None):
|
||||
return QModelIndex()
|
||||
|
||||
return self.createIndex(parentItem.row(), 0, parentItem)
|
||||
|
|
|
@ -178,13 +178,11 @@ class outlineItem(abstractItem):
|
|||
|
||||
def removeChild(self, row):
|
||||
r = abstractItem.removeChild(self, row)
|
||||
# Might be causing segfault when updateWordCount emits dataChanged
|
||||
self.updateWordCount(emit=False)
|
||||
self.updateWordCount()
|
||||
return r
|
||||
|
||||
def updateWordCount(self, emit=True):
|
||||
"""Update word count for item and parents.
|
||||
If emit is False, no signal is emitted (sometimes cause segfault)"""
|
||||
def updateWordCount(self):
|
||||
"""Update word count for item and parents."""
|
||||
if not self.isFolder():
|
||||
setGoal = F.toInt(self.data(self.enum.setGoal))
|
||||
goal = F.toInt(self.data(self.enum.goal))
|
||||
|
@ -219,12 +217,11 @@ class outlineItem(abstractItem):
|
|||
else:
|
||||
self.setData(self.enum.goalPercentage, "")
|
||||
|
||||
if emit:
|
||||
self.emitDataChanged([self.enum.goal, self.enum.setGoal,
|
||||
self.enum.wordCount, self.enum.goalPercentage])
|
||||
self.emitDataChanged([self.enum.goal, self.enum.setGoal,
|
||||
self.enum.wordCount, self.enum.goalPercentage])
|
||||
|
||||
if self.parent():
|
||||
self.parent().updateWordCount(emit)
|
||||
self.parent().updateWordCount()
|
||||
|
||||
def stats(self):
|
||||
wc = self.data(enums.Outline.wordCount)
|
||||
|
|
|
@ -293,16 +293,14 @@ class editorWidget(QWidget, Ui_editorWidget_ui):
|
|||
|
||||
try:
|
||||
self._model.dataChanged.connect(self.modelDataChanged, AUC)
|
||||
self._model.rowsInserted.connect(self.updateIndexFromID, AUC)
|
||||
self._model.rowsRemoved.connect(self.updateIndexFromID, AUC)
|
||||
#self.mw.mdlOutline.rowsAboutToBeRemoved.connect(self.rowsAboutToBeRemoved, AUC)
|
||||
self._model.rowsAboutToBeRemoved.connect(self.rowsAboutToBeRemoved, AUC)
|
||||
except TypeError:
|
||||
pass
|
||||
|
||||
self.updateStatusBar()
|
||||
|
||||
def setCurrentModelIndex(self, index=None):
|
||||
if index.isValid():
|
||||
if index and index.isValid():
|
||||
self.currentIndex = index
|
||||
self._model = index.model()
|
||||
self.currentID = self._model.ID(index)
|
||||
|
@ -313,17 +311,26 @@ class editorWidget(QWidget, Ui_editorWidget_ui):
|
|||
if self._model:
|
||||
self.setView()
|
||||
|
||||
def updateIndexFromID(self):
|
||||
def updateIndexFromID(self, fallback=None, ignore=None):
|
||||
"""
|
||||
Index might have changed (through drag an drop), so we keep current
|
||||
item's ID and update index. Item might have been deleted too.
|
||||
"""
|
||||
idx = self._model.getIndexByID(self.currentID)
|
||||
|
||||
# If we have an ID but the ID does not exist, it has been deleted
|
||||
It will ignore the passed model item to avoid ambiguity during times
|
||||
of inconsistent state.
|
||||
"""
|
||||
idx = self._model.getIndexByID(self.currentID, ignore=ignore)
|
||||
|
||||
# If we have an ID but the ID does not exist, it has been deleted.
|
||||
if self.currentID and idx == QModelIndex():
|
||||
# Item has been deleted, we open the parent instead
|
||||
self.setCurrentModelIndex(self.currentIndex.parent())
|
||||
# If we are given a fallback item to display, do so.
|
||||
if fallback:
|
||||
self.setCurrentModelIndex(fallback)
|
||||
else:
|
||||
# After tab closing is implemented, any calls to `updateIndexFromID`
|
||||
# should be re-evaluated to match the desired behaviour.
|
||||
raise NotImplementedError("implement tab closing")
|
||||
|
||||
# FIXME: selection in self.mw.treeRedacOutline is not updated
|
||||
# but we cannot simply setCurrentIndex through treeRedacOutline
|
||||
# because this might be a tab in the background / out of focus
|
||||
|
@ -337,19 +344,39 @@ class editorWidget(QWidget, Ui_editorWidget_ui):
|
|||
self.setView()
|
||||
|
||||
def modelDataChanged(self, topLeft, bottomRight):
|
||||
# if self.currentID:
|
||||
# self.updateIndexFromID()
|
||||
if not self.currentIndex:
|
||||
return
|
||||
if not self.currentIndex.isValid():
|
||||
return # Just to be safe.
|
||||
|
||||
# We are only concerned with minor changes to the current index,
|
||||
# so there is no need to call updateIndexFromID() nor setView().
|
||||
if topLeft.row() <= self.currentIndex.row() <= bottomRight.row():
|
||||
self.updateTabTitle()
|
||||
self.updateStatusBar()
|
||||
|
||||
#def rowsAboutToBeRemoved(self, parent, first, last):
|
||||
#if self.currentIndex:
|
||||
#if self.currentIndex.parent() == parent and \
|
||||
#first <= self.currentIndex.row() <= last:
|
||||
## Item deleted, close tab
|
||||
#self.mw.mainEditor.tab.removeTab(self.mw.mainEditor.tab.indexOf(self))
|
||||
def rowsAboutToBeRemoved(self, parent, first, last):
|
||||
if not self.currentIndex.isValid():
|
||||
return # Just to be safe.
|
||||
|
||||
# Look for a common ancestor to verify whether the deleted rows include our index in their hierarchy.
|
||||
childItem = self.currentIndex
|
||||
ancestorCandidate = childItem.parent() # start at folder above current item
|
||||
while (ancestorCandidate != parent):
|
||||
childItem = ancestorCandidate
|
||||
ancestorCandidate = childItem.parent()
|
||||
|
||||
if not ancestorCandidate.isValid():
|
||||
return # we ran out of ancestors without finding the matching QModelIndex
|
||||
|
||||
# My sanity advocates a healthy dose of paranoia. (Just to be safe.)
|
||||
if ancestorCandidate != parent:
|
||||
return # we did not find our shared ancestor
|
||||
|
||||
# Verify our origins come from the relevant first..last range.
|
||||
if first <= childItem.row() <= last:
|
||||
# If the row in question was actually moved, there is a duplicate item
|
||||
# already inserted elsewhere in the tree. Try to update this tab view,
|
||||
# but make sure we exclude ourselves from the search for a replacement.
|
||||
self.updateIndexFromID(fallback=parent, ignore=self.currentIndex.internalPointer())
|
||||
|
||||
def updateStatusBar(self):
|
||||
# Update progress
|
||||
|
@ -359,7 +386,7 @@ class editorWidget(QWidget, Ui_editorWidget_ui):
|
|||
if not mw:
|
||||
return
|
||||
|
||||
mw.mainEditor.updateStats()
|
||||
mw.mainEditor.tabChanged()
|
||||
|
||||
def toggleSpellcheck(self, v):
|
||||
self.spellcheck = v
|
||||
|
|
|
@ -383,7 +383,7 @@ class outlineBasics(QAbstractItemView):
|
|||
|
||||
parentItem.childItems.insert(index.row() + delta,
|
||||
parentItem.childItems.pop(index.row()))
|
||||
parentItem.updateWordCount(emit=False)
|
||||
parentItem.updateWordCount()
|
||||
|
||||
def moveUp(self): self.move(-1)
|
||||
def moveDown(self): self.move(+1)
|
||||
|
|
Loading…
Reference in a new issue