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
