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 nextVisibleWordEnd and prevVisibleWordEnd methods #808

Merged
merged 6 commits into from
Feb 12, 2019

Conversation

Galunid
Copy link
Member

@Galunid Galunid commented Feb 11, 2019

Add two methods that allow us to get the next/previous word from xpointer.

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick superficial nitpick. Nothing else jumps out but I'll leave the rest to @poire-z

cre.cpp Outdated Show resolved Hide resolved
cre.cpp Outdated Show resolved Hide resolved
cre.cpp Outdated Show resolved Hide resolved
@poire-z
Copy link
Contributor

poire-z commented Feb 11, 2019

Yeah, forget the tabs, stick to 4 spaces. There are still some functions with tabs, that we just haven't removed to not add unuseful commits.
And either keep the blank lines after the parameters checking and the one before the return parameters, or just drop the blank lines (for such little functions, you may as well remove these blank lines).

You may as well add now the prevVisibleWordStart and nextVisibleWordStart even if you don't use them yet.

cre.cpp Show resolved Hide resolved
cre.cpp Outdated
{"readDefaults", readDefaults},
{"saveDefaults", saveDefaults},
{"close", closeDocument},
{"__gc", closeDocument},
{NULL, NULL}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one stands out a bit. :-P

@poire-z
Copy link
Contributor

poire-z commented Feb 12, 2019

If you can fix that last tab before {NULL, NULL}, we can merge this PR, so you can then bump base in your frontend PR.
(I described how to do that on the frontend side in koreader/koreader#4314 (comment) in case you're not familiar with submodules.)

@poire-z poire-z merged commit 8005536 into koreader:master Feb 12, 2019
@poire-z
Copy link
Contributor

poire-z commented Feb 13, 2019

Looks like you need to fix (in your other PR) the 4 functions. For each of them:

     if (nodep.isNull())
         return 0;
-    nodep.nextVisibleWordEnd();
+    if (!nodep.nextVisibleWordEnd())
+        return 0;
     lua_pushstring(L, UnicodeToLocal(nodep.toString()).c_str());
     return 1;

Possibly only needed for the next* ones, but doing if for all is a good thing.
Because these nextVisibleWordEnd() return false when the move is not possible (while still moving the xpointer into a non-valid position - it becomes bad and corrupted when it return false, so it shouldn't be used for anything).

Noticable if you highlight some of the last text in a document, and move selection end ++ till you reach end of document. Next move will return false, but move the xpointer to the start of document, making moving back again impossible, highlighting all the document and koreader super slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants