davidhurka added a comment.

  In D15580#554079 <https://phabricator.kde.org/D15580#554079>, @simgunz wrote:
  
  > Should I configure the toolbar so that if the user changes to "text 
alongside icons" the toolbar appears like this?
  >  (the user can then expand the remaining icon-only buttons one by one by 
right clicking on them if he needs to visualize the text)
  
  
  I think that will be easier to tell as soon as a user decides to show the 
text, and complains about it.
  
  ---
  
  I looked at your code, and I it looks fine, as far as I am common with the 
Okular code. But I didn’t compile and test yet.
  
  I am wondering about //my// code. You merged both my pathes (D21755 
<https://phabricator.kde.org/D21755> and D21971 
<https://phabricator.kde.org/D21971>) into your development branch, right? 
Maybe you should rebase on D21971 <https://phabricator.kde.org/D21971>, so 
Phabricator (or gitlab) doesn’t amalganate unrelated changes.
  
  Besides that: How (well) does ToggleActionMenu work for you? I see that you 
don’t use suggestDefaultAction(). But if I got it right, Okular does not 
remember annotation tool selection across sessions, so it’s useless.

INLINE COMMENTS

> editannottooldialog.cpp:40
>  {
> +    m_builtinTool = builtinTool;
> +

Why initialize m_builtinTool to false in the initializer list? There aren’t 
superclass constructors or so which need it, or do I miss something?

> okular.upd:27
> +
> +# remove user-defined annotation tools to make space to new builtin 
> annotation tools
> +Id=annotation-toolbar

Is there a way to keep the user-defined annotations?

> shell.cpp:370
> +  toolBar( "mainToolBar" );
> +  KToggleToolBarAction * aToggleAnnotator = new KToggleToolBarAction( 
> toolBar( "annotationToolBar" ), i18n("&Annotations"), this );
> +  aToggleAnnotator->setIcon( QIcon::fromTheme( 
> QStringLiteral("draw-freehand") ) );

I remember you had struggle to find the annotation toolbar when you tried to 
show and hide it. Is this code related to the problem and did you solve the 
problem?

REPOSITORY
  R223 Okular

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

To: simgunz, #okular, #vdg
Cc: trickyricky26, simgunz, ltoscano, cfeck, aacid, davidhurka, knambiar, 
ngraham, tobiasdeiminger, okular-devel, johnzh, andisa, siddharthmanthan, 
maguirre, fbampaloukas, joaonetto, kezik, tfella, darcyshen

Reply via email to