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

Annotations support #843

Merged
merged 11 commits into from
Feb 18, 2020
Merged

Annotations support #843

merged 11 commits into from
Feb 18, 2020

Conversation

di72nn
Copy link
Member

@di72nn di72nn commented Oct 2, 2019

Currently implemented:

  • Saving annotations (when updating from server).
  • Methods to work with annotations: addAnnotation(), updateAnnotation() and deleteAnnotation() in OperationsHelper.
  • Offline changes and client-to-server sync.
  • Demo that displays a JS alert with JSON representation of article annotations in the ReadArticleActivity. See webview_htmlbase.html and JsAnnotationController for details.

What's not done:

  • Proper testing.
  • Any UI:
    • HTML/JS-side for highlighting and interacting with annotations in text.
    • Android side for modifications (only annotation text can be modified) and maybe more if not implemented in JS.

I'm not yet sure, but it seems that annotation changes on the server side do not mark articles as modified, so API clients never detect annotation changes. If that's correct, it may become an impediment akin to server-side deleted articles. Except this one doesn't have a workaround.

Everyone is welcome to step in to help with UI and JS stuff.

#115.

@di72nn
Copy link
Member Author

di72nn commented Oct 2, 2019

BTW, if anyone is interested, here are some code snippets I ran from ReadArticleActivicy using debug evaluation to test stuff:

AnnotationRange range = new AnnotationRange(null, null, "/ul[1]/li[1]", "/ul[1]/li[1]", 1, 5);
Annotation annotation = new Annotation(null, null, article.getId(), "Test annotation from app", null, null, null, null);
annotation.setRanges(Arrays.asList(range));
OperationsHelper.addAnnotation(this, article.getArticleId(), annotation);


fr.gaulupeau.apps.Poche.data.dao.entities.Annotation annotation = new ArrayList<>(article.getAnnotations()).get(1);
annotation.setText("Updated annotation from app");
OperationsHelper.updateAnnotation(this, article.getArticleId(), annotation);


OperationsHelper.deleteAnnotation(this, article.getArticleId(), new java.util.ArrayList<fr.gaulupeau.apps.Poche.data.dao.entities.Annotation>(article.getAnnotations()).get(1))

@Strubbl
Copy link
Contributor

Strubbl commented Nov 19, 2019

When having annotation support, maybe we can add a menu item to the navigation drawer, which is called "Annotated" or the like. It shall filter for annotated articles. With the tabs we can then see the unread, starred or achived annotated articles.

@di72nn
Copy link
Member Author

di72nn commented Nov 20, 2019

It sure is possible. However, a harder part is some kind of UI for displaying and creating annotations - without it this code is useless.

@di72nn
Copy link
Member Author

di72nn commented Feb 10, 2020

Rebased. Here's the old code just in case (github seems to save old commits, but I'm not bothered to check for how long).

Add annotations entities.
Extend updater logic to save annotations.
Add helper methods for changing annotations.
Support uploading annotation changes.
@di72nn
Copy link
Member Author

di72nn commented Feb 11, 2020

Here's a very clunky AnnotatorJS-based UI.
Short version: yes it kinda works, you can go ahead and test it.

Since I'm not a web developer at all, it was an absolute pain to do. Through sufferingI learned a lot.

For that reason I didn't even try to implement the JS part myself, so I had to resort to some libraries.

I considered using touch plugin, which only works with AnnotatorJS 1.2. I managed to get it running, but the UI didn't work that well with our webview (which always wraps the whole page instead of scrolling - the "annotate" button always appeared at the bottom of the article). I also couldn't build a proper version of AnnotatorJS 1.2 (some AnnotatorJS build scripts didn't work properly in my environment). Also I'm not very happy with using a version that ancient.

Instead I built AnnotatorJS's master. It is not mobile friendly, so I kludged it with Android's context menu action: you select text, press "Annotate" in the context menu, then annotator's "annotate" icon appears. That may be improved of course.
Another problem is that annotator popups often appear partly outside of the screen, not sure what's the deal with this.

Anyway, that last solution kinda works (UI wise; hopefully non-UI parts work ok).
So anyone interested in this feature should now have more than enough examples to properly implement the UI.

@di72nn
Copy link
Member Author

di72nn commented Feb 12, 2020

Fixed "Annotate" not doing anything on real devices.

Removed that extra step: annotation editor appears directly after pressing "Annotate".
Made a lousy attempt to horizontally center annotator popups.

AnnotatorJS is built from this branch (npm install ., make, take pkg/annotator.min.js).

Move estimated reading time and preview picture to header.
Escape more content.
Also slight refactoring.
@di72nn
Copy link
Member Author

di72nn commented Feb 13, 2020

Seeing the flurry of activity I presume this is the best it's gonna get (I'm certainly not improving it much more), so I'm marking annotations as an "experimental feature", disabling it by default and unlabeling PR's "work in progress" status.

@di72nn
Copy link
Member Author

di72nn commented Feb 13, 2020

Oh, I keep forgetting to thank @The-Compiler for #884 - remote WebView debugging was immensely useful, thank you!

@Strubbl
Copy link
Contributor

Strubbl commented Feb 14, 2020

I have installed this branch on my phone and i am going to test the feature.

@Strubbl Strubbl merged commit 1a10a6e into master Feb 18, 2020
@Strubbl Strubbl deleted the annotations branch February 18, 2020 20:13
@mirenbz
Copy link

mirenbz commented Aug 1, 2020

THANK YOU! This is a real deal breaker. Really hope it can be made better, but it works is certainly enough to keep (at least me) from switching. 😍

@mblennegard
Copy link

mblennegard commented Aug 31, 2020

I tried this out the other day and noticed that annotations made in the web application does not show up in the Android app.

I don't know if this is already reported elsewhere, but otherwise I can create a bug or feature request respectively.

@Strubbl
Copy link
Contributor

Strubbl commented Aug 31, 2020

I am not sure, but my guess is that annotations are only synced to the app after a "full update" and not the default "fast update". It is a similar problem with the tags, e.g. #463

@mblennegard
Copy link

@Strubbl You are absolutely right.
I just tried it and can confirm that a Full Update from the Android app indeed fetches the annotations made from the web application.

What's interesting is that any annotations made in the Android app syncs with the fast update, as the Android made annotations show up directly in the web application.
Or is it that any edits, such as setting tags, making annotations etc., made in the Android app syncs back to the DB immediately?

@di72nn
Copy link
Member Author

di72nn commented Sep 1, 2020

Created a new issue with some explanation: #1048.

Or is it that any edits, such as setting tags, making annotations etc., made in the Android app syncs back to the DB immediately?

Changes made in the app are always synced ASAP.

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

Successfully merging this pull request may close these issues.

4 participants