BUG: 358868
Details
- Reviewers
aacid ngraham - Group Reviewers
VDG - Commits
- R223:f788b5a384b5: Add action in Edit menu to select the text on current page
Click on "Select All Text on Current Page" entry in Edit menu to select the entire page. The selected text can then be copied via Edit menu item "Copy"
Diff Detail
- Repository
- R223 Okular
- Branch
- arcpatch-D18744
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 9529 Build 9547: arc lint + arc unit
what is a line or paragraph when the page has three columns of text? I don't think this makes sense
Personally i'm happy with a menu option, "no one" will see it, but that's the point, it's an option that "no one" needs.
And for the people that need it, they'll search for it, google it, maybe even try to use the documentation and then they'll find it in the menu.
And then people that for some reason need to select pages very often and are smart will learn how to assign the shortcut they want.
All right, let's do this. In final testing, I just found some blockers:
- If the current mouseMode is not TextSelect, then the menu item is enabled but does nothing. It should do its thing no matter what the mouseMode is, just like Select all does.
- The menu item is still enabled when Okular is opened without a document, and clicking on it crashes Okular. It needs to be disabled when no document is open.
No. I think that the action should be created in part.cpp, setEnabled(false) by default, then setEnabled(true) in the Part::openFile to avoid the crash.
Regretfully, no function (like isOpened) for setEnabled to connect to.
Another strategy can be to check for if doc is not opened and then setEnable(false)
Since setEnabled() is true by default, my if statement is basically dead code.
Now it is impossible to reach the Edit menu item at all.
Can you please try the version of this patch (just a minor tweak) that works fine for me? Thanks in advance for testing.
doc/index.docbook | ||
---|---|---|
1297 | Does this need updating? this doesn't depend on the mode anymore, does it? |
ui/pageview.cpp | ||
---|---|---|
5598 | Why this return? |
I just re-tested and now the menu item crashes when used, no matter which PDF I try. :(
Again, please test before submitting or updating a patch.
Try to fix the crash. I could not test it because for me I could not see the entry added to the menu.
You gotta fix that. :) Try deleting ~/.local/share/kxmlgui5/okular/partrc Does that help?
ui/pageview.cpp | ||
---|---|---|
5598 | If removed, it causes crash. |
ui/pageview.cpp | ||
---|---|---|
5598 | Sorry but that's not a valid answer, you'll have to explain why sometimes you have to delete area and why sometimes not |
@aacid area is a RegularAreaRect object which is a QRect bounding the selected text. "area" is to be deleted in all the cases except the case in which it is not possible to delete it since "area" will not contain any text in it and hence it will be nullptr. (case: if text is empty string) and hence return is called instead of deleting the "area".
Wait, stupid question: You assume area to be nullptr if there is no text on the page?
You do area = TextSelectionForItem( item );, PageView::TextSelectionForItem() unconditionally returns a Okular::RegularAreaRect * which it got from TextPage::TextArea(). TextPage::TextArea() does return new RegularAreaRect(); if TextSelectionForItem() is called on an empty page, and RegularAreaRect * ret= new RegularAreaRect; [complicated] return ret; [or] ret->addShape([...]); return ret otherwise.
TextSelectionForItem() tests whether it gets nullptr from TextArea(). But how can TextArea() return nullptr at all? new RegularAreaRect() vs. new RegularAreaRect?
ui/pageview.cpp | ||
---|---|---|
5587 | I don't think we should be copying to the clipboard. selectAll() isn't doing it, so this function doing it would be strage usability wise. |
Interestingly, setPageTextSelection() deletes area (without making it nullptr). Shouldn’t this side effect be documented (in another patch)?
ui/pageview.cpp | ||
---|---|---|
5587 | The other selection tools copy to clipboard. Maybe Select All should do so too, because the selection can not be used any other way than Menu -> Edit -> Copy. (Other way: e. g. right click -> Search With DuckDuckGo) |
Sure if you want to create a patch that documents it, feel free (BTW it can't set the pointer to null for the caller, that's not how pointers work)
ui/pageview.cpp | ||
---|---|---|
5587 | Not convinced, for example kate doesn't do that either. open kate, type something, press ctrl+a to select it, it only gets selected, not copied to the clipboard but if you select it with the mouse it then gets copied to the clipboard. It'd be the same we're doing, so i think this consistency is good to maintain. |