Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add highlight editing buttons #4582

Merged
merged 20 commits into from Feb 15, 2019

Conversation

Projects
None yet
7 participants
@Galunid
Copy link
Contributor

Galunid commented Feb 11, 2019

This allows user to move highlight end/beginning using buttons that appear after pressing on the highlight.
Screenshot:
image
This should be merged after koreader/koreader-base#808

Close #4555. Close #975. Provides a solution for #4134 and #4267 (comment)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 11, 2019

I don't really have time to look at the code atm, but could you replace the arrows with something more like in the skim to widget?

local button_chapter_prev = Button:new{
text = "│◁",

local button_chapter_next = Button:new{
text = '▷│',

@Frenzie Frenzie changed the title Add highlight edition buttons. Add highlight editing buttons Feb 11, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 11, 2019

Note that those are standard in the program. So just , ◁◁, , and ▷▷ should be fine.


@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 11, 2019

The symbols are a bit confusing. Looking at the code:
<< and >> move the start of the selection one word.
< and > move the end of the selection one word.

Can you see what you can do with ⇱ and ⇲ that could well represent the start and end of selection?

◁⇱      ⇱▷      ◁⇲       ⇲▷
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 12, 2019

@poire-z

The symbols are a bit confusing. Looking at the code:
<< and >> move the start of the selection one word.
< and > move the end of the selection one word.

Indeed. I completely misinterpreted them based on the screenshot. I thought it was something like one character vs. one word or one word vs. an entire sentence.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 12, 2019

Can you show a screenshot?
How would it be if you had just a single added line, with the 4 buttons in a row? Too small buttons?

self:updateHighlight(page, index, 0, 1)
end,
},
})

This comment has been minimized.

@poire-z

poire-z Feb 12, 2019

Contributor

Small thing. Can you shift the whole block 12 spaces left? The closing }) shoud vertically align with the start of table.insert(buttons, {.
Also the one with Delete|Edit, that does not need all that indentation now that you made it less nested.

This comment has been minimized.

@Galunid

Galunid Feb 12, 2019

Author Contributor

Sure, I'll do it once I get my emulator rebuild

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Feb 12, 2019

image
actually it's not bad

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 12, 2019

Indeed. I'd say go for the single row with 4 buttons if you like it too.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 12, 2019

Not 100% certain it will work for you, but here's how I'd do when things have evolved since you opened your PR and there are conflicts (just because you are lagging):

$ git checkout master
$ git fetch upstream
$ git merge upstream/master
$ git push origin master
$ git checkout issue-4555-highlight-buttons
$ git rebase origin/master
    First, rewinding head to replay your work on top of it...
    Applying: ...
$ git push -f origin issue-4555-highlight-buttons

@Galunid Galunid force-pushed the Galunid:issue-4555-highlight-buttons branch from 7564c5d to 1fea523 Feb 12, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 12, 2019

Just tested, looking and working nice.

But there's still a big issue: when you move the starting side, you can't edit (edit button) the bookmark anymore.
And when you move the end side, the edit button doesn't show the updated text, but still the old one. Same old text is also shown in the Bookmarks page.

It's a bit ugly that we save what's highlighted in 2 tables: bookmarks and highlight, as show in #3415 (comment).
So, you may have to update the clone in bookmarks too.
Does not look easy, you first need to find it :|
You may look at readerbookmark.lua. There's a ReaderBookmark:renameBookmark that launch a dialog, but you may pick stuff from it and create a ReaderBookmark:updateBookmark(item, from_highlight), counterpart of your updateHighlight()...

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Feb 12, 2019

@poire-z :

Completely off-topic, but, IIRC,

git fetch upstream
git rebase upstream/master
...
git push -f

(git rebase -i upstream/master if you want to squash/edit things along the way).

Should be enough ;).

That implies having set-up an upstream remote for the fetch, and working in a tracked branch for the "anonymous" push.

I personally have a git-rbranch alias in my shell to create a local+remote tracked branch, because apparently there wasn't a one-liner to do that the first time I needed it ;p.

# Create a remote tracking branch...
function git-rbranch() {
        if (( ${ARGC} < 1 )) ; then
                echo "Branch name not specified!"
        fi
        local new_branch="${1}"

        # Create it locally
        git checkout -b "${new_branch}"
        # Push it
        git push origin "${new_branch}"
        # Set up tracking info
        git branch -u "origin/${new_branch}" "${new_branch}"
}

And once that's merged, if that was from my master branch, I resync it the hard way:

git fetch upstream
git reset --hard upstream/master
git submodule update
git push -f
@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Feb 13, 2019

I've created the counterpart method, the only problem I have is how to get text between two xpointers. I didn't find function that does this in credocument.lua. I wrote(more like hacked ;)) one. The question is whether we need to add it to the koreader-base or not. Probably there already exists something like this.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 13, 2019

I wrote(more like hacked ;)) one. The question is whether we need to add it to the koreader-base

Show us :)
credocument/cre.cpp indeed miss a getTextFromXPointers among:

    {"getTextFromXPointer", getTextFromXPointer},
    {"getHTMLFromXPointer", getHTMLFromXPointer},
    {"getHTMLFromXPointers", getHTMLFromXPointers},

So you probably need to add one (unless you found a hack), which looks not that hard (some parts from getTextFromPositions() like ldomXRange r(startp, endp); selText = r.getRangeText() should work).

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Feb 13, 2019

static int getTextFromXPointers(lua_State *L) {
	CreDocument *doc = (CreDocument*) luaL_checkudata(L, 1, "credocument");
	const char* pos0 = luaL_checkstring(L, 2);
	const char* pos1 = luaL_checkstring(L, 3);

	LVDocView *tv = doc->text_view;
	ldomDocument *dv = doc->dom_doc;

	ldomXPointer startp = dv->createXPointer(lString16(pos0));
	ldomXPointer endp = dv->createXPointer(lString16(pos1));
	if (!startp.isNull() && !endp.isNull()) {
	    lua_newtable(L); // new text boxes
		ldomXRange r(startp, endp);
		if (r.getStart().isNull() || r.getEnd().isNull())
			return 0;
		r.sort();

		bool not_panning = r.getStart() == r.getEnd();
		if ( (not_panning || r.getStart().isVisibleWordChar()) && !r.getStart().isVisibleWordStart())
			r.getStart().prevVisibleWordStart();
		if ( (not_panning || r.getEnd().isVisibleWordChar()) && !r.getEnd().isVisibleWordEnd())
			r.getEnd().nextVisibleWordEnd();
		if (r.isNull())
			return 0;

		if (r.getStart() == r.getEnd()) { // for single CJK character
			ldomNode * node = r.getStart().getNode();
			lString16 text = node->getText();
			int textLen = text.length();
			int offset = r.getEnd().getOffset();
			if (offset < textLen - 1)
				r.getEnd().setOffset(offset + 1);
		}

		lString16 selText = r.getRangeText( '\n', 8192 );
		lua_pushstring(L, UnicodeToLocal(selText).c_str());
	    return 1;
	}
    return 0;
}

This works, I pretty much copied getTextFromPositions, I'm not sure whether it can be simplified.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 13, 2019

Looks alright, that's what I would have come up with :)
Just some cleanup: no need for LVDocView *tv = doc->text_view and the 5 lines about not_panning .
I guess this function generically should trust the provided xpointers, and should not attempt to move them to align them to words.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 15, 2019

@poire-z Squash & merge will take care of that. Only a conflict is a problem, and one that needs to be solved manually (which @Galunid did).

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Feb 15, 2019

OT: is there a simple way to check when user holds a button?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 15, 2019

Otoh that's called hold_callback on a button. That is, you can always easily add a hold "gesture" to anything, but most of the widgets come with predefined properties that are used if set.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 15, 2019

You would not need to check for that. You would just need to provide a hold_callback = to your buttons.
Would need a simple patch in a widget:

--- a/frontend/ui/widget/buttontable.lua
+++ b/frontend/ui/widget/buttontable.lua
@@ -53,6 +53,7 @@ function ButtonTable:init()
                 text = btn_entry.text,
                 enabled = btn_entry.enabled,
                 callback = btn_entry.callback,
+                hold_callback = btn_entry.hold_callback,
                 width = (self.width - sizer_space)/column_cnt,
                 max_width = (self.width - sizer_space)/column_cnt - 2*self.sep_width - 2*self.padding,
                 bordersize = 0,

There is yet an issue I can't pinpoint quickly, it's that the hold would then still be processed by the buttondialog (by MovableContainer) and make it translucid...

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 15, 2019

Sorry, I didn't realize the question was about a ButtonTable. :-)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 15, 2019

There is yet an issue I can't pinpoint quickly, it's that the hold would then still be processed by the buttondialog (by MovableContainer) and make it translucid...

Shouldn't you just return true in your handler to keep it from propagating?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 15, 2019

It's supposed to already do that:

function Button:onHoldSelectButton()
if self.enabled and self.hold_callback then
self.hold_callback()
elseif self.hold_input then
self:onInput(self.hold_input)
elseif type(self.hold_input_func) == "function" then
self:onInput(self.hold_input_func())
end
return true
end

But MovableContainer is tricky and hacky... I'll have a look later today.

Btw @Galunid, if you're going on with this, have a try at adding a table.insert(buttons, {}) before the one with your arrow buttons, it would add a horizontal separator (no current way to add a vertical separator between the buttons about start of selection and the ones about end of it alas). Dunno if it looks better or not, your choice.

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Feb 15, 2019

image
I'd say it looks better without separator. I've pretty much finished "move by character" part of code. Could you please notify me when you figure out/fix what's wrong with MovableContainer?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 15, 2019

@poire-z Btw, this looks like it's probably ready to squash & merge?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 15, 2019

Dunno if @Galunid wants to go on in here with his pretty much finished move by char (as he already pushed some commits after his go for merge), but we have till this european evening to merge and have that in tomorrow's nightly.

About the MovableContainer issue, this looks like enough to solve it:

--- a/frontend/ui/widget/button.lua
+++ b/frontend/ui/widget/button.lua
@@ -102,6 +102,13 @@ function Button:init()
                     range = self.dimen,
                 },
                 doc = "Hold Button",
+            },
+            -- Safe-guard for when used inside a MovableContainer
+            HoldReleaseSelectButton = {
+                GestureRange:new{
+                    ges = "hold_release",
+                    range = self.dimen,
+                },
             }
         }
     end
@@ -240,4 +247,16 @@ function Button:onHoldSelectButton()
     return true
 end

+function Button:onHoldReleaseSelectButton()
+    -- Safe-guard for when used inside a MovableContainer,
+    -- which would handle HoldRelease and process it like
+    -- a Hold if we wouldn't return true here
+    if self.enabled and self.hold_callback then
+        return true
+    elseif self.hold_input or type(self.hold_input_func) == "function" then
+        return true
+    end
+    return false
+end
+
 return Button

This will still allow moving it when holding on the non-hold-enabled button like Delete & Edit.

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Feb 15, 2019

It seems to make no difference

            {
                text = _("◁⇱"),
                callback = function()
                    self:updateHighlight(page, index, 0, -1, false)
                end,
                hold_callback = function()
                    logger.dbg("detected hold!!")
                    self:updateHighlight(page, index, 0, -1, true)
                    return true
                end
            }

"detected hold!!" never gets logged

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 15, 2019

Have you added the + hold_callback = btn_entry.hold_callback, to buttontable.lua suggested above?
This works for me:

+            {
+                text = _("blah"),
+                callback = function()
+                    self:updateHighlight(page, index, 1, 1)
+                end,
+                hold_callback = function()
+                    self:updateHighlight(page, index, 1, 2)
+                end,
+            }

galunid added some commits Feb 15, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 15, 2019

Looks and works allright.
I initially thought tap would move by char, and hold would move by word (the longer the press, the longer the move).
But it feels like people would most often move by word - and @ptrm (#4267 (comment)) should be fine with one or two long-press to grab the next punctuation.

Thanks for the quick last changes. Looks good to me.
(Now, going to look for all the issues this PR closes :) edit: added to first post.)

@Galunid

This comment has been minimized.

Copy link
Contributor Author

Galunid commented Feb 15, 2019

Cool, hopefully there are not too many ;)

@poire-z poire-z merged commit 015fb4d into koreader:master Feb 15, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Galunid Galunid deleted the Galunid:issue-4555-highlight-buttons branch Feb 15, 2019

@arooni

This comment has been minimized.

Copy link

arooni commented Feb 15, 2019

Wow this is amazing !! Koreader keeps getting better and better !

@ptrm

This comment has been minimized.

Copy link

ptrm commented Feb 16, 2019

and @ptrm (...) should be fine with one or two long-press to grab the next punctuation.

Absolutely :) Love the feature

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 17, 2019

I'll push something later today to prevent start and end to pass each other.

@seacatwu

This comment has been minimized.

Copy link

seacatwu commented Feb 17, 2019

I think the edit menu below the first highlight will be more useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.