<table><tr><td style="">davidhurka 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/D21971">View Revision</a></tr></table><br /><div><div><p>To answer your second comment first, and to be sure you understand everything how I understand you understand it...</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/D21971#559081" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21971#559081</a>, <a href="https://phabricator.kde.org/p/simgunz/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@simgunz</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>I also would like to get rid of <tt style="background: #ebebeb; font-size: 13px;">DefaultLogic</tt> and just use <tt style="background: #ebebeb; font-size: 13px;">ImplicitDefaultAction</tt>, but I am not sure if this creates problem when <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> is placed in a menu.</p></div>
</blockquote>
<p>No, creates no problem.</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>Should it [<tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt>] behave as a standard <tt style="background: #ebebeb; font-size: 13px;">KActionMenu</tt> when in a menu? I think that when in a menu there is no meaning of "default action", because when clicked, the <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> should just open the submenu, right?</p></blockquote>
<p>Yes, there it should behave as a plain submenu. And yes, in a submenu there is no thing like default action.</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/D21971#559080" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21971#559080</a>, <a href="https://phabricator.kde.org/p/simgunz/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@simgunz</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>I have tried to analyze all the possible cases regarding <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> placed in a toolbar.</p>
<p>From what I understand:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">DefaultLogic</tt> means, the developer needs to manage everything manually calling/connecting <tt style="background: #ebebeb; font-size: 13px;">setDefaultAction</tt>.</li>
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">ImplicitDefaultAction</tt> instead allows to set the default action automatically when an action is triggered</li>
</ul></div>
</blockquote>
<p>Exactly.</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>CASE 1</p>
<p><tt style="background: #ebebeb; font-size: 13px;">QToolButton::MenuButtonPopup</tt> or <tt style="background: #ebebeb; font-size: 13px;">QToolButton::DelayedPopup</tt> and <tt style="background: #ebebeb; font-size: 13px;">m_menuLogic == DefaultLogic</tt></p>
<p>If I do not call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt> and no action is selected:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">the <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> icon is shown</li>
<li class="remarkup-list-item">Clicking on the toolbar button nothing happens [weird, but maybe ok]</li>
<li class="remarkup-list-item">Checking an action has no effect on the toolbar button</li>
<li class="remarkup-list-item">The toolbar button is never shown as checked [weird]</li>
</ul></blockquote>
<p>That’s not an intended usage. When you choose <tt style="background: #ebebeb; font-size: 13px;">DefaultLogic</tt> mode, you have to explicitely set the default action, like with a plain <tt style="background: #ebebeb; font-size: 13px;">QToolButton</tt>.</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>If I do call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt> and no action is selected:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">the toolbar button icon is set to the one of the suggested default action</li>
<li class="remarkup-list-item">Clicking on the toolbarbutton always activates the suggested default action</li>
<li class="remarkup-list-item">Checking a different action has no effect on points 1 and 2 [weird]</li>
</ul></blockquote>
<p>Yes, in <tt style="background: #ebebeb; font-size: 13px;">DefaultLogic</tt> mode you have to update the default action yourself.</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>CASE 2</p>
<p><tt style="background: #ebebeb; font-size: 13px;">QToolButton::MenuButtonPopup</tt> or <tt style="background: #ebebeb; font-size: 13px;">QToolButton::DelayedPopup</tt> and <tt style="background: #ebebeb; font-size: 13px;">m_menuLogic == ImplicitDefaultAction</tt><br />
If I do call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt> and no action is selected everything works as expected.<br />
If I do not call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt> and no action is selected:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">the <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> icon is shown</li>
<li class="remarkup-list-item">Clicking on the toolbar button nothing happens [weird, but I think it is how QToolButton is supposed to work]</li>
</ul></blockquote>
<p>Correct.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">Checking an action makes it the default [ok]</li>
<li class="remarkup-list-item">The toolbar button is shown as checked when an action is selected [ok]
<br /><br />
CASE 3 [& 4]
<br /><br />
<tt style="background: #ebebeb; font-size: 13px;">QToolButton::InstantPopup</tt> [...] [weird]</li>
</ul></blockquote>
<p>Yes, <tt style="background: #ebebeb; font-size: 13px;">InstantPopup</tt> is totally pointless for <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt>.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;">
<hr class="remarkup-hr" />
<p>I suggest to change <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu::defaultAction</tt> to:</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);">QAction * ToggleActionMenu::defaultAction()
{
if ( ( m_menuLogic & ImplicitDefaultAction ) )
{
QAction * aChecked = checkedAction( menu() );
if ( aChecked )
{
return aChecked;
}
}
return m_defaultAction;
}</pre></div>
<p>and get rid of <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu::suggestDefaultAction</tt>.</p>
<p>In this way we simplify the things and in <tt style="background: #ebebeb; font-size: 13px;">ImplicitDefaultAction</tt> mode it is enough to check the action after the <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> has been created to make it the default action automatically.</p></blockquote>
<p>(That is already possible.)</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>In <tt style="background: #ebebeb; font-size: 13px;">PageView::setupActions</tt> we should have the following:</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);">d->aMouseModeMenu = new ToggleActionMenu( QIcon(),QString(), this,
QToolButton::MenuButtonPopup,
ToggleActionMenu::ImplicitDefaultAction );
d->aMouseModeMenu->addAction( d->aMouseSelect );
d->aMouseModeMenu->addAction( d->aMouseTextSelect );
d->aMouseModeMenu->addAction( d->aMouseTableSelect );
d->aMouseModeMenu->setDefaultAction( d->aMouseTextSelect );
...
d->aMouseSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::RectSelect );
d->aMouseTextSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TextSelect );
d->aMouseTableSelect->setChecked( Okular::Settings::mouseMode() == Okular::Settings::EnumMouseMode::TableSelect );</pre></div>
<p>what do you think?</p></blockquote>
<p>Should be probably ok, that’s just not really “implicit”.<br />
I don’t know where <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> will be used elsewhere, and how the actions will be pre-checked there. Probably there will be usecases without a definite time to pre-check the actions.</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>I suggest this change and adding an exception for case 4 in the constructor.</p></blockquote>
<p>I don’t know about exceptions yet. Are they this stuff that crashes the application when something goes slightly wrong and the caller doesn’t care?</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>Dear User,<br />
*MOOOP MOOOP* Unhandled Exception 0x11050!!!()65567 *MOOOP MOOOP*<br />
Lang.Excep.ion.Blah.blah<br />
[ Ok ]</p></blockquote>
<p>In that case: No, I don’t think it makes sense to crash an application because a QToolButton has the wrong default action, but the user can still use the menu...</p></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/D21971">https://phabricator.kde.org/D21971</a></div></div><br /><div><strong>To: </strong>davidhurka<br /><strong>Cc: </strong>ngraham, simgunz, okular-devel, johnzh, andisa, siddharthmanthan, maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen, aacid<br /></div>