Changeset View
Standalone View
kdevplatform/shell/textdocument.cpp
| Show First 20 Lines • Show All 80 Lines • ▼ Show 20 Line(s) | |||||
| 81 | public: | 81 | public: | ||
| 82 | explicit TextDocumentPrivate(TextDocument *textDocument) | 82 | explicit TextDocumentPrivate(TextDocument *textDocument) | ||
| 83 | : q(textDocument) | 83 | : q(textDocument) | ||
| 84 | { | 84 | { | ||
| 85 | } | 85 | } | ||
| 86 | 86 | | |||
| 87 | ~TextDocumentPrivate() | 87 | ~TextDocumentPrivate() | ||
| 88 | { | 88 | { | ||
| 89 | delete addedContextMenu; | 89 | // handle case we are deleted while context menu is not yet hidden | ||
| 90 | addedContextMenu = nullptr; | 90 | // we want to remove all actions we added, especially those not owned by the document | ||
| 91 | // (i.e. created on-the-fly during ContextMenuExtension::populateMenu with ownership | ||||
| 92 | // set to our addedContextMenu) but owned by the plugins | ||||
rjvbb: This can be made easier to understand:
`Handle the case we are being deleted while the context… | |||||
| 93 | cleanContextMenu(true); | ||||
| 91 | 94 | | |||
| 92 | saveSessionConfig(); | 95 | saveSessionConfig(); | ||
| 93 | delete document; | 96 | delete document; | ||
| 94 | } | 97 | } | ||
| 95 | 98 | | |||
| 96 | void setStatus(KTextEditor::Document* document, bool dirty) | 99 | void setStatus(KTextEditor::Document* document, bool dirty) | ||
| 97 | { | 100 | { | ||
| 98 | QIcon statusIcon; | 101 | QIcon statusIcon; | ||
| ▲ Show 20 Lines • Show All 120 Lines • ▼ Show 20 Line(s) | 208 | { | |||
| 219 | // not retrieveable from the VCS. If that is not the case, then the document can safely be | 222 | // not retrieveable from the VCS. If that is not the case, then the document can safely be | ||
| 220 | // reloaded without displaying a dialog asking the user. | 223 | // reloaded without displaying a dialog asking the user. | ||
| 221 | if ( dirty ) { | 224 | if ( dirty ) { | ||
| 222 | queryCanRecreateFromVcs(document); | 225 | queryCanRecreateFromVcs(document); | ||
| 223 | } | 226 | } | ||
| 224 | setStatus(document, dirty); | 227 | setStatus(document, dirty); | ||
| 225 | } | 228 | } | ||
| 226 | 229 | | |||
| 230 | void cleanContextMenu(bool forceDeleteNow = false) | ||||
| 231 | { | ||||
| 232 | if (!addedContextMenu) { | ||||
| 233 | return; | ||||
| 234 | } | ||||
| 235 | | ||||
| 236 | if (currentContextMenu) { | ||||
| 237 | const auto actions = addedContextMenu->actions(); | ||||
| 238 | for (QAction* action : actions) { | ||||
| 239 | currentContextMenu->removeAction(action); | ||||
| 240 | } | ||||
| 241 | currentContextMenu.clear(); | ||||
| 242 | } | ||||
| 243 | | ||||
| 244 | // the addedContextMenu owns actions created on-the-fly for the context menu | ||||
| 245 | // and thuse deletes them as well on destruction. | ||||
| 246 | // As some actions potentially could be connected to triggered-signal handlers | ||||
| 247 | // using Qt::QueuedConnection (at least SwitchToBuddyPlugin does so currently) | ||||
| 248 | // deleting them here also deletes the connection before the handler is triggered | ||||
| 249 | // so by default we delay the destruction to next eventloop as well | ||||
As above: // The addedContextMenu owns the actions (it doesn't have any other actions, right?) // Some actions could potentially be connected [...] does so currently) rjvbb: As above:
`// The addedContextMenu owns the actions` (it doesn't have any other actions, right? | |||||
No, to confuse everyone it does have other actions which it does not own: some plugins also have persistent QAction objects, e.g. used for the app main menu, which they also reuse for the context menu. Trying to reword to make this more clear here. kossebau: No, to confuse everyone it does have other actions which it does not own: some plugins also… | |||||
| 250 | if (forceDeleteNow) { | ||||
| 251 | delete addedContextMenu; | ||||
| 252 | } else { | ||||
| 253 | addedContextMenu->deleteLater(); | ||||
anthonyfieroni: Why not deleteLater in any case? | |||||
Originall I wanted to use delete here only, and added the deleteLater as (temporary) work-around for the one known and any potentially 3rd-party action which has a Qt::QueuedConnection connect. The latter I see as questionable thing, as with context menus there is no promise t will exist any longer once the menu is closed, and the actions here are added to the context menu also passing ownership to it. Something I plan to fix afterwards, but did not want to mess with additionally, and also not change behaviour in the 5.3 branch. In general I also personally disfavour using deleteLater, as it makes it harder to track as human the life-time management of objects and the order in which they are deleted, making debugging more complicated. A deleteLater also runs into troubles on shutdown, if all document objects & other stuff is deleted, where there might be no more event loop to reach afterwards. Think especially unit tests. Which is why in case of TextDocument destructor already now I propose to use delete. But given current unit tests are now running into issues with the need for extra eventloop hit for clean shutdown, and runtime also showed no issues, changed now to just always deleteLater. Private note made to revisit this once some patch to fix the SwitchToBuddy actions is done and accepted :) kossebau: Originall I wanted to use delete here only, and added the deleteLater as (temporary) work… | |||||
Yes, deleteLater() introduces a kind of object management as found in ObjC and Cocoa (refcounting and autorelease pools). What's missing in C++ is something that allows an object to cancel its own deleting when it detects that it is still in use (to avoid issues with deletion order). I haven't checked the code, but if deleteLater() causes the target object to be deleted when the current eventloop exits I would expect that this also happens when the last eventloop exits (and gets deleted). It's less clear what happens if you call the method after the main eventloop stopped: If deleteLater() is called after the main event loop has stopped, the object will not be deleted. Since Qt 4.8, if deleteLater() is called on an object that lives in a thread with no running event loop, the object will be destroyed when the thread finishes. You'd say that those 2 statements contradict each other. I did find something quite clever in the QSingleShotTimer ctor: connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);. Anyway, I have run into multiple issues with window (and menu) objects being deleted directly on Mac, because of pending events which then got delivered to a stale (C++ or private ObjC) object. Hence the official Qt advice to use deleteLater() on UI element instances. Here it might be safe not to since the object(s) in question shouldn't be displayed at this point (I think?). rjvbb: Yes, `deleteLater()` introduces a kind of object management as found in ObjC and Cocoa… | |||||
| 254 | } | ||||
| 255 | addedContextMenu = nullptr; | ||||
| 256 | } | ||||
| 257 | | ||||
| 227 | TextDocument * const q; | 258 | TextDocument * const q; | ||
| 228 | 259 | | |||
| 229 | QPointer<KTextEditor::Document> document; | 260 | QPointer<KTextEditor::Document> document; | ||
| 230 | IDocument::DocumentState state = IDocument::Clean; | 261 | IDocument::DocumentState state = IDocument::Clean; | ||
| 231 | QString encoding; | 262 | QString encoding; | ||
| 232 | bool loaded = false; | 263 | bool loaded = false; | ||
| 233 | // we want to remove the added stuff when the menu hides | 264 | // we want to remove the added stuff when the menu hides | ||
| 234 | QMenu* addedContextMenu = nullptr; | 265 | QMenu* addedContextMenu = nullptr; | ||
| 266 | QPointer<QMenu> currentContextMenu; | ||||
| 235 | }; | 267 | }; | ||
| 236 | 268 | | |||
| 237 | class TextViewPrivate | 269 | class TextViewPrivate | ||
| 238 | { | 270 | { | ||
| 239 | public: | 271 | public: | ||
| 240 | explicit TextViewPrivate(TextView* q) : q(q) {} | 272 | explicit TextViewPrivate(TextView* q) : q(q) {} | ||
| 241 | 273 | | |||
| 242 | TextView* const q; | 274 | TextView* const q; | ||
| ▲ Show 20 Lines • Show All 472 Lines • ▼ Show 20 Line(s) | |||||
| 715 | } | 747 | } | ||
| 716 | 748 | | |||
| 717 | void KDevelop::TextDocument::textChanged(KTextEditor::Document *document) | 749 | void KDevelop::TextDocument::textChanged(KTextEditor::Document *document) | ||
| 718 | { | 750 | { | ||
| 719 | Q_UNUSED(document); | 751 | Q_UNUSED(document); | ||
| 720 | notifyContentChanged(); | 752 | notifyContentChanged(); | ||
| 721 | } | 753 | } | ||
| 722 | 754 | | |||
| 723 | void KDevelop::TextDocument::populateContextMenu( KTextEditor::View* v, QMenu* menu ) | 755 | void KDevelop::TextDocument::unpopulateContextMenu() | ||
| 724 | { | 756 | { | ||
| 725 | Q_D(TextDocument); | 757 | Q_D(TextDocument); | ||
| 726 | 758 | | |||
| 727 | if (d->addedContextMenu) { | 759 | auto* menu = qobject_cast<QMenu*>(sender()); | ||
| 728 | const auto actions = d->addedContextMenu->actions(); | 760 | Q_ASSERT(menu == d->currentContextMenu); | ||
Why take the risk of hard to understand behaviour in production builds? IMO just make this a runtime if if you cannot prove right here and now that the ASSERT is redundant. The gain of skipping the if is purely academic (and failing the test doesn't justify causing an abort in debug builds either; a qWarning() would do just fine). rjvbb: Why take the risk of hard to understand behaviour in production builds? IMO just make this a… | |||||
Left over from debug/experiment setup, removed now as it serves no real purpose, as this should just work and for the case of two menus at the same time there would be the check in populateContextMenu() to catch this. kossebau: Left over from debug/experiment setup, removed now as it serves no real purpose, as this should… | |||||
| 729 | for (QAction* action : actions) { | 761 | | ||
| 730 | menu->removeAction(action); | 762 | disconnect(menu, &QMenu::aboutToHide, this, &TextDocument::unpopulateContextMenu); | ||
| 731 | } | 763 | | ||
| 732 | delete d->addedContextMenu; | 764 | d->cleanContextMenu(); | ||
| 733 | } | 765 | } | ||
| 734 | 766 | | |||
| 767 | void KDevelop::TextDocument::populateContextMenu( KTextEditor::View* v, QMenu* menu ) | ||||
| 768 | { | ||||
| 769 | Q_D(TextDocument); | ||||
| 770 | | ||||
| 771 | Q_ASSERT(!d->addedContextMenu); | ||||
rjvbb: Idem, just call unpopulateContextMenu(). | |||||
Indeed, qCWarning is better in here as long-term solution, where the Q_ASSERT only served me to quickly get kicked while drafting the code. kossebau: Indeed, qCWarning is better in here as long-term solution, where the Q_ASSERT only served me to… | |||||
Thanks for confirming what I already expected about the presence of certain of those statements :) rjvbb: Thanks for confirming what I already expected about the presence of certain of those statements… | |||||
| 772 | | ||||
| 773 | d->currentContextMenu = menu; | ||||
| 774 | connect(menu, &QMenu::aboutToHide, this, &TextDocument::unpopulateContextMenu); | ||||
| 775 | | ||||
| 735 | d->addedContextMenu = new QMenu(); | 776 | d->addedContextMenu = new QMenu(); | ||
| 736 | 777 | | |||
| 737 | EditorContext c(v, v->cursorPosition()); | 778 | EditorContext c(v, v->cursorPosition()); | ||
| 738 | auto extensions = Core::self()->pluginController()->queryPluginsForContextMenuExtensions(&c, d->addedContextMenu); | 779 | auto extensions = Core::self()->pluginController()->queryPluginsForContextMenuExtensions(&c, d->addedContextMenu); | ||
| 739 | 780 | | |||
| 740 | ContextMenuExtension::populateMenu(d->addedContextMenu, extensions); | 781 | ContextMenuExtension::populateMenu(d->addedContextMenu, extensions); | ||
| 741 | 782 | | |||
| 742 | const auto actions = d->addedContextMenu->actions(); | 783 | const auto actions = d->addedContextMenu->actions(); | ||
| ▲ Show 20 Lines • Show All 57 Lines • Show Last 20 Lines | |||||
This can be made easier to understand:
`Handle the case we are being deleted while the context is not yet hidden.
We want to` [...]
Move the but owned by the plugins to before the explanation in parentheses.