<table><tr><td style="">simgunz 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/D21755">View Revision</a></tr></table><br /><div><div><p>I have tested this patch and added some inline comments. It seems to me that ToggleActionMenu requires way more external code to make it work, compared to ToolAction, which is quite automated. I think that some things could be made default in ToggleActionMenu, and some aspect hidden as well e.g. manage the QActionGroup internally without having the user have to manage it and set the action eveytime.</p>

<p>I tried it by only adding the actions to it and it does not work, no menu is shown. Adding the signal connection is also not enough. After that I gave up for now (I am lazy). But I think it needs to be a little more user-friendly.</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/D21755#inline-123555">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.cpp:660</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">aMouseModeMenu</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">ToggleActionMenu</span><span class="p">(</span> <span style="color: #aa4000">this</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">aMouseModeMenu</span><span style="color: #aa2211">-></span><span class="n">setDelayed</span><span class="p">(</span> <span style="color: #304a96">false</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">aMouseModeMenu</span><span style="color: #aa2211">-></span><span class="n">setStickyMenu</span><span class="p">(</span> <span style="color: #304a96">false</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">setDelayed and setStickyMenu shoule be defaults, without having to set them 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/D21755#inline-123556">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.cpp:667</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">ac</span><span style="color: #aa2211">-></span><span class="n">addAction</span><span class="p">(</span> <span class="n">QStringLiteral</span><span class="p">(</span> <span style="color: #766510">"mouse_selecttools"</span> <span class="p">),</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">aMouseModeMenu</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">connect</span><span class="p">(</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">aMouseModeMenu</span><span style="color: #aa2211">-></span><span class="n">menu</span><span class="p">(),</span> <span style="color: #aa2211">&</span><span class="n">QMenu</span><span style="color: #aa2211">::</span><span class="n">triggered</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">             <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">aMouseModeMenu</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">ToggleActionMenu</span><span style="color: #aa2211">::</span><span class="n">setDefaultAction</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Can't this connection be defined inside ToggleActionMenu?</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/D21755#inline-123557">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.cpp:671</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #74777d">// Use the current mouse mode action as default action, or Text Selection by default.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QAction</span><span style="color: #aa2211">*</span> <span class="n">enabledMouseModeAction</span> <span style="color: #aa2211">=</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">mouseModeActionGroup</span><span style="color: #aa2211">-></span><span class="n">checkedAction</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">if</span> <span class="p">(</span> <span class="n">enabledMouseModeAction</span> <span style="color: #aa2211">&&</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">aMouseModeMenu</span><span style="color: #aa2211">-></span><span class="n">menu</span><span class="p">()</span><span style="color: #aa2211">-></span><span class="n">actions</span><span class="p">().</span><span class="n">contains</span><span class="p">(</span> <span class="n">enabledMouseModeAction</span> <span class="p">)</span> <span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">All the code below, cannot be managed inside ToggleActionMenu?</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/D21755#inline-123553">View Inline</a><span style="color: #4b4d51; font-weight: bold;">toggleactionmenu.h:47</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #a0a000">public</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">explicit</span> <span class="n">ToggleActionMenu</span><span class="p">(</span> <span class="n">QWidget</span> <span style="color: #aa2211">*</span><span class="n">parent</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">ToggleActionMenu</span><span class="p">(</span> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">text</span><span class="p">,</span> <span class="n">QWidget</span> <span style="color: #aa2211">*</span> <span class="n">parent</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Make parent a QObject to make it more general. I would use this in AnnotationActionHandler which is not a QWidget.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R223 Okular</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D21755">https://phabricator.kde.org/D21755</a></div></div><br /><div><strong>To: </strong>davidhurka, Okular<br /><strong>Cc: </strong>simgunz, aacid, VDG, okular-devel, fbampaloukas, joaonetto, tfella, ngraham, darcyshen<br /></div>