<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/D21971">View Revision</a></tr></table><br /><div><div><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>
<h4 class="remarkup-header">Case 1</h4>
<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>
<ol 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>
</ol>
<p>If I do call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt> and no action is selected:</p>
<ol 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>
</ol>
<h4 class="remarkup-header">Case 2</h4>
<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>
<ol 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>
<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]</li>
</ol>
<h4 class="remarkup-header">Case 3</h4>
<p><tt style="background: #ebebeb; font-size: 13px;">QToolButton::InstantPopup</tt> and <tt style="background: #ebebeb; font-size: 13px;">m_menuLogic == DefaultLogic</tt><br />
If I do not call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt> everything works as expected<br />
If I do call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt>: <strong>(should raise an exception)</strong></p>
<ul class="remarkup-list">
<li class="remarkup-list-item">The button icon is set to that of the suggested action and stays as so forever [weird]</li>
</ul>
<h4 class="remarkup-header">Case 4</h4>
<p><tt style="background: #ebebeb; font-size: 13px;">QToolButton::InstantPopup</tt> and <tt style="background: #ebebeb; font-size: 13px;">m_menuLogic == ImplicitDefaultAction</tt> <strong>(should raise an exception)</strong><br />
If I do not call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt>:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">The button icon is set to that of the checked action [weird]</li>
</ul>
<p>If I do call <tt style="background: #ebebeb; font-size: 13px;">suggestDefaultAction</tt>:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">The button icon is set to that of the suggested action [weird]</li>
<li class="remarkup-list-item">The button icon is set to that of the checked action [weird]</li>
</ul>
<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>
<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>
<p>I suggest this change and adding an exception for case 4 in the constructor.</p>
<p>I have not yet tested what happens if you place <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu</tt> in a menu and if my suggested change breaks the things.</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>