<table><tr><td style="">kossebau marked 4 inline comments as done.<br />kossebau added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D22424">View Revision</a></tr></table><br /><div><div><p>Thanks for first review & testing.</p>
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D22424#494980" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D22424#494980</a>, <a href="https://phabricator.kde.org/p/rjvbb/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@rjvbb</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>It seems that it achieves its primary goal (on Linux, haven't tested OS X but context menus don't use platform menu APIs so should work the same).</p></div>
</blockquote>
<p>Sounds good so far.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>However, the patch has a side-effect: I'm getting messages like this each time I switch or open documents:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">kdevplatform.shell: Broken text-document: QUrl("file:///opt/local/var/tmp/kdevelop-tmp-505-%7Bf793a513-f14e-47b9-8448-06ca72900c04%7D/kdevelop_Q27955.patch")</pre></div>
<p>This example is for a file that indeed doesn't exist, but that I also do not have open (so WTH?!) but I also get it for files that open perfectly.</p>
<p>I fail to see an immediate reason for this, so I'm back to running my own patch.</p></blockquote>
<p>Interesting. Not seen here, but giving this some stress testing now. The kdevelop_xxxxx.patch is coming from the diff review system creating text document objects for displaying the diffs, but I also see no direct relation ship to this patch here. Will poke around a bit,</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22424#inline-126502">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">textdocument.cpp:244-249</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">As above:</p>
<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">// The addedContextMenu owns the actions</tt> (it doesn't have any other actions, right?)<br />
<tt style="background: #ebebeb; font-size: 13px;">// and thus deletes them on destruction</tt> ("as well" doesn't really add anything here).</p>
<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">// Some actions could potentially be connected</tt> [...] <tt style="background: #ebebeb; font-size: 13px;">does so currently)</tt><br />
<tt style="background: #ebebeb; font-size: 13px;">// Deleting them here would delete the connection</tt> [...]<br />
<tt style="background: #ebebeb; font-size: 13px;">// so we delay their destruction to the next eventloop by default.</tt> (I don't see any other instances that are deleteLater'ed "as well").</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">Trying to reword to make this more clear here.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22424#inline-126506">View Inline</a><span style="color: #4b4d51; font-weight: bold;">anthonyfieroni</span> wrote in <span style="color: #4b4d51; font-weight: bold;">textdocument.cpp:253</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Why not deleteLater in any case?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">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 :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22424#inline-126514">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">textdocument.cpp:760</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Why take the risk of hard to understand behaviour in production builds? IMO just make this a runtime <tt style="background: #ebebeb; font-size: 13px;">if</tt> 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).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D22424#inline-126515">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">textdocument.cpp:771</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Idem, just call unpopulateContextMenu().</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D22424">https://phabricator.kde.org/D22424</a></div></div><br /><div><strong>To: </strong>kossebau, KDevelop<br /><strong>Cc: </strong>anthonyfieroni, rjvbb, kdevelop-devel, hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd<br /></div>