<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/D21195">View Revision</a></tr></table><br /><div><div><p>Okular does not follow the KDELibs coding style, so I tried to follow the existing coding style. Maybe I didn’t understand something right, if so, please correct me.</p>
<p>Currently, all actions are enabled permanently. Should they be disabled if there is no document shown, like the View Mode menu?</p>
<p>Additionally, it’s not possible to assign shortcuts, so this would break Bug 407217 even more. :(<br />
When a shortcut is added via Configure Shortcuts, it only works once, then it disappears. After a restart of Okular, it works fine. What’s going on? How can I fix that?</p>
<p>There are two actions "Change Colors" now in the toolbar configuration dialog, is that OK?<br />
<a href="https://phabricator.kde.org/F6821950" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F6821950: after_two_change_colors.png</a></p>
<p>If this menu should be inserted to the menubar, I suggest this position. Probably it’s bad to make a submenu checkable:<br />
<a href="https://phabricator.kde.org/F6821947" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">F6821947: after.png</a></p>
<p>For more Request For Comment [RFC], see my inline comments.</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/D21195#inline-118909">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.cpp:616</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(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">QAction</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span class="n"><span class="bright">aToggle</span>ChangeColors</span> <span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">new</span></span><span class="bright"> </span><span class="n"><span class="bright">QA</span>ction<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">i18n</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"&Toggle Change Colors"</span></span><span class="bright"></span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">this</span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">ac</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">addAction</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QStringLiteral</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"toggle_change_colors"</span></span><span class="bright"></span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span class="n"><span class="bright">aToggleChangeColor</span>s</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">connect</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span class="n"><span class="bright">aToggle</span>ChangeColors<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span class="n"><span class="bright">QAction</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">triggered</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">this</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span class="n"><span class="bright">PageView</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">slotToggleChangeColors</span></span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// </span>Change<span class="bright"> </span>Colors <span class="bright">a</span>ction<span class="bright"> menu</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">aChangeColorsMenu</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">new</span></span><span class="bright"> </span><span class="n"><span class="bright">KActionMenu</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span class="n"><span class="bright">QIcon</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">fromTheme</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span class="n"><span class="bright">QStringLiteral</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"color-management"</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span class="n"><span class="bright">i18n</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span style="color: #766510"><span class="bright">"Change Colors"</span></span><span class="bright"> </span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">thi</span>s</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">a</span>ChangeColors<span class="bright">Menu</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">setDelayed</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span style="color: #304a96"><span class="bright">false</span></span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Which would be a good accelerator for “Change Colors”?</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/D21195#inline-118910">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.cpp:637</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">QActionGroup</span> <span style="color: #aa2211">*</span> <span class="n">cmGroup</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QActionGroup</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 style="color: #304a96">#define ADD_COLORMODE_ACTION( action, name, id ) \</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">do { \</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I want to avoid function-like macros. Can I just add a private method instead?</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/D21195#inline-118911">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.cpp:672</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">aChangeColorsMenu</span><span style="color: #aa2211">-></span><span class="n">addSeparator</span><span class="p">();</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">aConfigure</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QAction</span><span class="p">(</span> <span class="n">i18nc</span><span class="p">(</span> <span style="color: #766510">"@item:inmenu color mode"</span><span class="p">,</span> <span style="color: #766510">"C&onfigure..."</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">aChangeColorsMenu</span><span style="color: #aa2211">-></span><span class="n">addAction</span><span class="p">(</span> <span class="n">aConfigure</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Unicode ellipsis … instead of periods ... ?</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/D21195#inline-118914">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.cpp:5585</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">// Assigning a shortcut to a color mode only makes sense if it can toggle the feature on and off.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">slotToggleChangeColors</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This toggles Change Colors on and off, if the same shortcut e. g. for Invert Colors is triggered twice. But it also toggles if an already selected mode is clicked in the menu. Is that OK?</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/D21195#inline-118908">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pageview.h:75</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; "> <span style="color: #aa4000">void</span> <span style="color: #004012">setupBaseActions</span><span class="p">(</span> <span class="n">KActionCollection</span> <span style="color: #aa2211">*</span> <span class="n">collection</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">void</span> <span style="color: #004012">setupViewerActions</span><span class="p">(</span> <span class="n">KActionCollection</span> <span style="color: #aa2211">*</span> <span class="n">collection</span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">void</span> <span style="color: #004012">setupViewerActions</span><span class="p">(</span> <span class="n">KActionCollection</span> <span style="color: #aa2211">*</span> <span class="n">collection<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">Okular</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">Part</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span class="n"><span class="bright">part</span></span> <span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">void</span> <span style="color: #004012">setupActions</span><span class="p">(</span> <span class="n">KActionCollection</span> <span style="color: #aa2211">*</span> <span class="n">collection</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Adding a link to Part is neccessary, because Part controls m_embedMode, which is used to access the configuration dialog.</p>
<p style="padding: 0; margin: 8px;">However, the whole Change Colors menu is probably better in Part instead of PageView. Currently, every PageView creates a new menu, and with multible tabs their checked states get out of sync.</p>
<p style="padding: 0; margin: 8px;">m_embedMode gives another question: should I check m_embedMode before creating this menu? In the print preview mode, Change Colors does not make much sense.</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/D21195">https://phabricator.kde.org/D21195</a></div></div><br /><div><strong>To: </strong>davidhurka, Okular, VDG<br /><strong>Cc: </strong>ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid<br /></div>