D18744: Add action in Edit menu to select the current page

[Shubham]

shubham edited the test plan for this revision.

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg
Cc: michaelweghorn, kde-doc-english, davidhurka, abetts, loh.tar, alexde, 
ngraham, okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham edited the test plan for this revision.

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg
Cc: michaelweghorn, kde-doc-english, davidhurka, abetts, loh.tar, alexde, 
ngraham, okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Michael Weghorn]

michaelweghorn added a comment.


  In D18744#412409 , @michaelweghorn 
wrote:
  
  > The test plan still says that Ctrl+P will select the page ("or press 
combination of CTRL and P to select the entire page"). As far as I understand 
the previous comments, this is Alt+P now, isn't it? (I didn't do any tests.) 
Can you update the description accordingly?
  
  
  And as far as I can see, the same applies for the docbook.

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg
Cc: michaelweghorn, kde-doc-english, davidhurka, abetts, loh.tar, alexde, 
ngraham, okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Michael Weghorn]

michaelweghorn added a comment.


  The test plan still says that Ctrl+P will select the page ("or press 
combination of CTRL and P to select the entire page"). As far as I understand 
the previous comments, this is Alt+P now, isn't it? (I didn't do any tests.) 
Can you update the description accordingly?

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg
Cc: michaelweghorn, kde-doc-english, davidhurka, abetts, loh.tar, alexde, 
ngraham, okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham retitled this revision from "[RFC]Add action in Edit menu to select the 
current page" to "Add action in Edit menu to select the current page".

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham updated this revision to Diff 51685.
shubham marked 3 inline comments as done.
shubham retitled this revision from "Add action in Edit menu to select the 
current page " to "Add action in Edit menu to select the current page".
shubham added a comment.


  Add action to part.rc
  Bump version to 40

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18744?vs=51610&id=51685

BRANCH
  arcpatch-D18744

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

AFFECTED FILES
  doc/index.docbook
  part.rc
  ui/pageview.cpp
  ui/pageview.h

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[loh tar]

loh.tar added inline comments.

INLINE COMMENTS

> shubham wrote in pageview.cpp:743
> So better I will remove that.

Here is e.g. Alt-P, and a couple of F-Keys not used. 
But perhaps exist in other software some similar function with a usual key 
sequence that can be adopt

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham added inline comments.

INLINE COMMENTS

> ngraham wrote in pageview.cpp:743
> It doesn't have a shortcut at all. If you can't think of anything that 
> doesn't conflict, then just don't give it one.

So better I will remove that.

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> shubham wrote in pageview.cpp:743
> What sequence should I use?

It doesn't have a shortcut at all. If you can't think of anything that doesn't 
conflict, then just don't give it one.

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham added inline comments.

INLINE COMMENTS

> ngraham wrote in pageview.cpp:743
> Erm, that's the shortcut used for printing. Did you test this?

What sequence should I use?

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Nathaniel Graham]

ngraham added a comment.


  In D18744#411621 , @shubham wrote:
  
  > I knew ctrl p is for print, so for time being I kept it so I can get 
suggestion for other shortcut sequence.
  
  
  It's not acceptable to deliberately publish a diff that does the wrong thing 
purely for the purpose of soliciting comments without adding `[WIP]` or `[RFC]` 
to the title. What if nobody noticed this and it landed as-is and broke 
printing?
  
  > Also, I can't tell why entry is not added into Edit menu even though I had 
added that in docbook.
  
  The docbook is just for documentation. To put it in the menu structure, you 
need to add the action to `part.rc` and bump the version number at the top of 
the file.

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham added a comment.


  In D18744#411599 , @ngraham wrote:
  
  > Please test your changes. The new menu is not actually added to the Edit 
menu and the shortcut you chose conflicts with the print shortcut.
  
  
  I knew ctrl p is for print, so for time being I kept it so I can get 
suggestion for other shortcut sequence. Also, I can't tell why entry is not 
added intk Edit menu even though I had added that in docbook

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  Please test your changes. The new menu is not actually added to the Edit menu 
and the shortcut you chose conflicts with the print shortcut.

INLINE COMMENTS

> pageview.cpp:740
> +QAction *selectCurrentPage = new QAction( this );
> +selectCurrentPage->setIcon( QIcon::fromTheme( QStringLiteral( 
> "edit-select-all" ) ) );
> +ac->addAction( QStringLiteral("Select Current Page"), selectCurrentPage 
> );

This icon is already used for the Select All action, and we don't want to 
re-use the same icon for two actions in the same menu. Let's remove it.

> pageview.cpp:741
> +selectCurrentPage->setIcon( QIcon::fromTheme( QStringLiteral( 
> "edit-select-all" ) ) );
> +ac->addAction( QStringLiteral("Select Current Page"), selectCurrentPage 
> );
> +connect( selectCurrentPage, &QAction::triggered, this, 
> &PageView::slotSelectPage );

The correct text here would be "Select all text on current page"

> pageview.cpp:743
> +connect( selectCurrentPage, &QAction::triggered, this, 
> &PageView::slotSelectPage );
> +ac->setDefaultShortcut(selectCurrentPage, QKeySequence(Qt::CTRL + 
> Qt::Key_P));
> +addAction( selectCurrentPage );

Erm, that's the shortcut used for printing. Did you test this?

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg, ngraham
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham edited the test plan for this revision.

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid

D18744: Add action in Edit menu to select the current page

[Shubham]

shubham retitled this revision from "Add action in Edit menu to select the 
entire page " to "Add action in Edit menu to select the current page ".

REPOSITORY
  R223 Okular

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

To: shubham, aacid, #vdg
Cc: kde-doc-english, davidhurka, abetts, loh.tar, alexde, ngraham, 
okular-devel, gennad, tfella, skadinna, darcyshen, aacid