D15580: [WIP] New annotation toolbar

[Nathaniel Graham]

ngraham added a comment.


  In D15580#554079 , @simgunz wrote:
  
  > I have never seen a close button on a standard toolbar. Are you sure it is 
a correct design pattern?
  
  
  This isn't a standard toolbar; it's a toolbar that can be shown and hidden. 
Once it's shown, it's not obvious how to hide it--especially now that it shows 
itself after using a quick annotation tool. That'll make it appear, but it 
won't be obvious how to make it go away again if it doesn't have an integrated 
close button.
  
  Even though the HIG recommends against it, I think it's fine to change the 
button style here since we're quite space-constrained.
  
  Expanding spacers were added in 4357ef235ecb8b8b71ca0867d6cfc02acf292fae 
.

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

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added inline comments.

INLINE COMMENTS

> davidhurka wrote in shell.cpp:370
> 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?

>> @simgunz just wrote:
> 
> https://phabricator.kde.org/D15580#544534

That’s what I thought about. :)

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

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  >> I think there's only one remaining thing I noticed, then I'm ready to give 
it a UI stamp of approval: the toolbar should have a close button on the 
far-right side (you can use an expanding spacer to position it there) so people 
don't have to go up to the menubar to close the toolbar once they're done using 
it.
  
  
  
  > I have never seen a close button on a standard toolbar. Are you sure it is 
a correct design pattern?
  
  One more thing regarding this. kpartgui.dtd does not have a "spacer" item. So 
that and the close action should be inserted programmatically with some 
non-so-nice code to retrieve the annotation toolbar (see 
https://phabricator.kde.org/D15580#544534).
  
  @davidhurka I am going to reply to your comments Sunday

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

D15580: [WIP] New annotation toolbar

[David Hurka]

davidhurka added a comment.


  In 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 
 and D21971 
) into your development branch, right? 
Maybe you should rebase on 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(""), 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

[okular] [Bug 396087] Okular stops rendering some pages, locks up at 25% CPU usage and won't die

[Albert Astals Cid]

https://bugs.kde.org/show_bug.cgi?id=396087

--- Comment #11 from Albert Astals Cid  ---
I'm 93% sure this is a duplicate of 396137

-- 
You are receiving this mail because:
You are the assignee for the bug.

[okular] [Bug 413304] Chinese characters instead of Polish diacritical marks

[Albert Astals Cid]

https://bugs.kde.org/show_bug.cgi?id=413304

Albert Astals Cid  changed:

   What|Removed |Added

 Status|NEEDSINFO   |REPORTED
 Resolution|WAITINGFORINFO  |---

--- Comment #11 from Albert Astals Cid  ---
Sadly, needs someone to be able to reproduce it before being able to fix it

-- 
You are receiving this mail because:
You are the assignee for the bug.

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz added a comment.


  In D15580#553421 , @ngraham wrote:
  
  > All right, that works!
  >
  > I think there's only one remaining thing I noticed, then I'm ready to give 
it a UI stamp of approval: the toolbar should have a close button on the 
far-right side (you can use an expanding spacer to position it there) so people 
don't have to go up to the menubar to close the toolbar once they're done using 
it.
  
  
  I have never seen a close button on a standard toolbar. Are you sure it is a 
correct design pattern?
  
  There are cases where the close button might be misleading given the 
configurability of the toolbars.
  
  F7669753: Screenshot_20191025_210846.png 

  
  //Annotation toolbar in the same row as the main toolbar. There is not 
distinction between the two, so it is not clear what the close button applies 
to.//
  
  F7669750: Screenshot_20191025_211307.png 

  
  //Having the button right aligned may put it in a weird position when the 
window is maximized.//
  
  --
  
  Regarding the HIG guidelines 
. We are currently 
violating the following two rules
  
  1. Don’t change the button style from the default, which is text beside icons.
  2. Don’t hide toolbars by default. If a toolbar can be hidden, users should 
easily be able to make the toolbar viewable again.
  
  Is this fine?
  
  My opinion:
  
  1. Don't care much. "Icon-only" is fine for me.
  2. I think it makes sense to hide it by default. It is consistent with the 
current Okular behavior, so users that are used to the current toolbar will 
find no difference.
  
  -
  
  Now the default is "icon-only". If a user changes to "text alongside icons" 
the toolbar will overflow even on a large monitor. 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)
  
  F7670023: Screenshot_20191025_222139.png 

  //Non-obvious actions expanded, obvious one compressed.//

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

D15580: [WIP] New annotation toolbar

[Simone Gaiarin]

simgunz updated this revision to Diff 68768.
simgunz added a comment.


  - Warn user that stamps in PDF are an experimental feature

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15580?vs=68650=68768

BRANCH
  new-annotation-toolbar_ToggleActionMenu

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

AFFECTED FILES
  CMakeLists.txt
  conf/dlgannotations.cpp
  conf/dlgannotationsbase.ui
  conf/editannottooldialog.cpp
  conf/editannottooldialog.h
  conf/okular.kcfg
  okular.upd
  part.cpp
  part.rc
  shell/shell.cpp
  shell/shell.rc
  ui/annotationactionhandler.cpp
  ui/annotationactionhandler.h
  ui/annotationwidgets.cpp
  ui/annotationwidgets.h
  ui/data/CMakeLists.txt
  ui/data/tools.xml
  ui/data/toolsQuick.xml
  ui/pageview.cpp
  ui/pageview.h
  ui/pageviewannotator.cpp
  ui/pageviewannotator.h
  ui/pageviewutils.cpp
  ui/pageviewutils.h
  ui/side_reviews.cpp
  ui/toggleactionmenu.cpp
  ui/toggleactionmenu.h
  ui/toolaction.cpp
  ui/toolaction.h

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

[okular] [Bug 413304] Chinese characters instead of Polish diacritical marks

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=413304

--- Comment #10 from pyj...@protonmail.com ---
(In reply to Albert Astals Cid from comment #9)
> Do you get any warning if you start okular from the command line (i.e.
> konsole)?

No, I don't get any error but when I open test.markdown file i get every time
strange message:
`discarding "Send sms via KDE Connect" ("ShareUrl")`

-- 
You are receiving this mail because:
You are the assignee for the bug.