<table><tr><td style="">igorkushnir marked 2 inline comments as done.<br />igorkushnir 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/D3040" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>I guess everyone is OK with Ctrl+0 working only when the documentation widget has focus. This widget actually gets focus on scrolling, even though individual windows on my system don't get focus in such cases. So this should not be a problem.</p>

<p>I've noticed an issue with my own shortcut implementation: the shortcut does not work in the Programmer Dvorak layout, because this layout requires pressing Shift to access digits. It also does not work with numpad 0. So I'd like to replace <tt style="background: #ebebeb; font-size: 13px;">event->modifiers() == Qt::ControlModifier</tt> with <tt style="background: #ebebeb; font-size: 13px;">(event->modifiers() & Qt::ControlModifier)</tt>. This will reset zoom level even if Ctrl+Shift+0 or Ctrl+Alt+0 is pressed. Please let me know if this is likely to cause conflicts with some global shortcuts. If it is, I could write even more specific code: <tt style="background: #ebebeb; font-size: 13px;">(event->modifiers() & Qt::ControlModifier) && !(event->modifiers() & ~(Qt::ControlModifier | Qt::ShiftModifier | Qt::KeypadModifier))</tt>.</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/D3040#inline-18864" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">standarddocumentationview.cpp:84</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Not my review, but FWIW, that's turning a function that only does something the 1st time into one that either crashes or else leaks ZoomController instances.</p>

<p style="padding: 0; margin: 8px;">IMHO asserts like this should only be used for conditions that cannot be handled gracefully regardless of whether they should never happen.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This code mimics what <a href="https://code.woboq.org/qt5/qtbase/src/widgets/kernel/qwidget.cpp.html#_ZN7QWidget12insertActionEP7QActionS1_" class="remarkup-link" target="_blank" rel="noreferrer">QWidget::insertAction</a> does with a nullptr parameter.</p>

<p style="padding: 0; margin: 8px;"><a href="https://phabricator.kde.org/p/mwolff/" style="
              border-color: #f1f7ff;
              color: #19558d;
              background-color: #f1f7ff;
                border: 1px solid transparent;
                border-radius: 3px;
                font-weight: bold;
                padding: 0 4px;" rel="noreferrer">@mwolff</a>, please confirm that you still prefer assertion. I'll make the change then.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R33 KDevPlatform</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D3040" rel="noreferrer">https://phabricator.kde.org/D3040</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>igorkushnir, mwolff, KDevelop<br /><strong>Cc: </strong>rjvbb, mwolff, apol, kdevelop-devel<br /></div>