<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><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#560532" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21971#560532</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);"><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#560495" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21971#560495</a>, <a href="https://phabricator.kde.org/p/davidhurka/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@davidhurka</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><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#559485" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21971#559485</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>To sum it up, if I understand correctly:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">we can remove <tt style="background: #ebebeb; font-size: 13px;">InstantPopup</tt> given that we can use <tt style="background: #ebebeb; font-size: 13px;">KActionMenu</tt> or <tt style="background: #ebebeb; font-size: 13px;">KSelectAction</tt> to provide that use case (so no need to raise exceptions)</li>
</ul></div>
</blockquote>
<p>Yes, but how? It can’t simply be forbidden in the constructor argument list. Remove it from the body of the constructor, so KActionMenu is set to MenuButtonPopup instead?</p></div>
</blockquote>
<p>Create a new enum <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu::ToolButtonPopupMode</tt> with only <tt style="background: #ebebeb; font-size: 13px;">DelayedPopup</tt> and <tt style="background: #ebebeb; font-size: 13px;">MenuButtonPopup</tt>.</p></div>
</blockquote>
<p>Sounds reasonable. Can we be sure that no one wants to use <tt style="background: #ebebeb; font-size: 13px;">InstantPopup</tt>, because <tt style="background: #ebebeb; font-size: 13px;">KSelectAction</tt> does that? KSelectAction does not plug in a menu, right? I can’t think of such a use case, even the View Mode menu (see <a href="https://phabricator.kde.org/D21196" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21196</a>) wouldn’t fit.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>The change to <tt style="background: #ebebeb; font-size: 13px;">ToggleActionMenu::defaultAction</tt> is required, otherwise if <tt style="background: #ebebeb; font-size: 13px;">setDefaultAction</tt> is called on action1 manually and then action2 is checked, <tt style="background: #ebebeb; font-size: 13px;">defaultAction</tt> returns action1.</p></blockquote>
<p>I can’t follow you here. Yes, when in DefaultLogic mode, and setDefaultAction(action1) was called, defaultAction() returns action1. Maybe your thinking was focused on checkable actions so far?</p></blockquote></blockquote>
<p>I think I understood now what you meant. Your suggested changes to the method defaultAction():</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 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></div>
</blockquote>
<p>I thought whether this could give unexpected results, when you trigger an action so it becomes unchecked, but there are more checkable options further down in the menu. (Imagine Color Mode from <a href="https://phabricator.kde.org/D21195" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21195</a>, with an additional option <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Preserve images</span></span></span>.) But then, you wouldn’t use <tt style="background: #ebebeb; font-size: 13px;">ImplicitDefaultAction</tt> mode.</p>
<p>But there are more general use cases where this does not work, like the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Create an additive primitive</span></span></span> action menu in FreeCAD. This could be created with a ToggleActionMenu in ImplicitDefaultAction mode, by simply adding all actions and suggesting/setting a default one.<br />
<a href="https://phabricator.kde.org/F7752159" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F7752159: image.png</a><br />
The last triggered action is not neccessarily checked, especcialy ones which are “actions”, not “options” and thus not checkable.</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>Try the following code and steps:</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>
<ul class="remarkup-list">
<li class="remarkup-list-item">Check <tt style="background: #ebebeb; font-size: 13px;">Browse Mode</tt></li>
<li class="remarkup-list-item">Restart Okular Results: <tt style="background: #ebebeb; font-size: 13px;">Browse Mode</tt> is checked / <tt style="background: #ebebeb; font-size: 13px;">Text selection</tt> is the default action and unchecked</li>
<li class="remarkup-list-item">Check <tt style="background: #ebebeb; font-size: 13px;">Text selection</tt></li>
<li class="remarkup-list-item">Restart Okular Results: <tt style="background: #ebebeb; font-size: 13px;">Text selection</tt> is the default action and checked</li>
<li class="remarkup-list-item">Check <tt style="background: #ebebeb; font-size: 13px;">Table Selection</tt></li>
<li class="remarkup-list-item">Restart Okular Results: <tt style="background: #ebebeb; font-size: 13px;">Text selection</tt> is the default action and unchecked, <tt style="background: #ebebeb; font-size: 13px;">Table Selection</tt> is checked but not the default action
<br /><br />
With my change the last case will result in: Results: <tt style="background: #ebebeb; font-size: 13px;">Text selection</tt> is the default action and checked</li>
</ul></blockquote>
<p>Yes, QAction::setChecked() does not emit triggered(), but toggled(). So theoreticaly I should connect ToggleActionMenu::setDefaultAction() to toggled(), but that’s not emitted by QMenu, so I have to connect to all actions in the menu manually. Furthermore, an action could be toggled from outside ToggleActionMenu’s menu, e. g. by using the shortcut. It may or may not be intended that this updates the toolbar button. When the application itself toggles an action, it is usually not intended that the toolbar buttons are updated.</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>