D21195: [RFC] Create a Change Colors menu (with toolbar button)

[David Hurka]

davidhurka added a comment.


  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.
  
  Currently, all actions are enabled permanently. Should they be disabled if there is no document shown, like the View Mode menu?
  
  Additionally, it’s not possible to assign shortcuts, so this would break Bug 407217 even more. :(
  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?
  
  There are two actions "Change Colors" now in the toolbar configuration dialog, is that OK?
  F6821950: after_two_change_colors.png <https://phabricator.kde.org/F6821950>
  
  If this menu should be inserted to the menubar, I suggest this position. Probably it’s bad to make a submenu checkable:
  F6821947: after.png <https://phabricator.kde.org/F6821947>
  
  For more Request For Comment [RFC], see my inline comments.

INLINE COMMENTS

> pageview.cpp:616
> +    // Change Colors action menu
> +    d->aChangeColorsMenu = new KActionMenu( QIcon::fromTheme( QStringLiteral("color-management") ), i18n( "Change Colors" ), this );
> +    d->aChangeColorsMenu->setDelayed( false );

Which would be a good accelerator for “Change Colors”?

> pageview.cpp:637
> +    QActionGroup * cmGroup = new QActionGroup( this );
> +#define ADD_COLORMODE_ACTION( action, name, id ) \
> +do { \

I want to avoid function-like macros. Can I just add a private method instead?

> pageview.cpp:672
> +    d->aChangeColorsMenu->addSeparator();
> +    QAction * aConfigure = new QAction( i18nc( "@item:inmenu color mode", "C&onfigure..." ), this );
> +    d->aChangeColorsMenu->addAction( aConfigure );

Unicode ellipsis … instead of periods ... ?

> pageview.cpp:5585
> +        // Assigning a shortcut to a color mode only makes sense if it can toggle the feature on and off.
> +        slotToggleChangeColors();
> +    }

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?

> pageview.h:75
>          void setupBaseActions( KActionCollection * collection );
> -        void setupViewerActions( KActionCollection * collection );
> +        void setupViewerActions( KActionCollection * collection, Okular::Part * part );
>          void setupActions( KActionCollection * collection );

Adding a link to Part is neccessary, because Part controls m_embedMode, which is used to access the configuration dialog.

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.

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.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D21195

To: davidhurka, #okular, #vdg
Cc: ngraham, okular-devel, joaonetto, tfella, darcyshen, aacid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20190513/d09574c6/attachment-0001.html>