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

Refactoring & some features #931

Merged
merged 78 commits into from
May 8, 2020
Merged

Refactoring & some features #931

merged 78 commits into from
May 8, 2020

Conversation

di72nn
Copy link
Member

@di72nn di72nn commented Mar 8, 2020

I started fixing bad fields usage in QueueItem, one thing led to another, and it seems I'm fixing #880 (I actually did, but there's still a lot of redundancy to remove).

The PR includes #930, but I'll leave it be for the time being.

I'll add some explanation later.

@di72nn di72nn force-pushed the service_reorganization branch 2 times, most recently from 519c423 to 2a6334e Compare March 10, 2020 13:16
@di72nn
Copy link
Member Author

di72nn commented Mar 10, 2020

Time for some details.

As was mentioned in #930, the use of generic fields is not exactly obvious. Unfortunately, greenDAO doesn't support entity inheritance, so I added (6779550) "view" classes that encapsulate operation-specific logic and provide better API to the QueueItem users. I left generic fields (in QueueItem and DB) as is, but they are not for direct use anymore.

I wanted to deal with similar problem in ActionRequest, but it required more changes, so the rewriting commenced.

One of the most prominent mistake I made when I originally wrote all that offline synchronization stuff was that some DB writing was still happening in the main (UI) thread (#880). Even if the operations are tiny, they get stuck if there are other background writes (background sync, etc.) for the whole duration of those background transactions.
Those long operations should be shortened of course (like doing the sync in a bunch of short transactions), but I'll leave it for another day. It won't solve the problem completely anyway.

So I decided to move all DB writing from UI. Most of the operations can be scheduled using serializableparcelable objects with a simple IntentService, but arguments to some operations (like updating article tags and annotations) are a real pain to make parcelable. Turns out it is possible to pass non-parcelable objects to a bound service. So a TaskService was born - a hybrid of a "started" service (lifecycle doesn't depend on clients) and a "bound" service (allows method calls with non-parcelable parameters).

A lot of refactoring followed: implementation of better parcelable tasks and move of the majority of old operations to these tasks, splitting of old services into separate classes and some other stuff.

I also rewrote "offline queue" optimization: previously it was removing/merging related changes during addition to queue, now it is done during queue sync. This allows to add new queue items even if there's a sync in progress (new items won't be processed right away, but neither they'll break anything). This also removed a lot of code duplication.

The codebase still leaves a lot to be desired, but it's way better than before (for one, you can follow an operation from its entry point (in OperationsHelper) to the code that carries out the operation without having any detours to the task execution code).

Oh, there's one problem though: the changes may be delayed. For example, if there's a long sync in the background and a user marks an article as favorite and then presses "mark as read", in the lists the article will still appear neither favorited nor read until the background operation is finished. Still an improvement over ANR. Improving long operations should help.

I plan to do some more changes, but everything should be already working.

@di72nn
Copy link
Member Author

di72nn commented Mar 10, 2020

@tcitworld can you shed some light on release plans? It is problematic to write new stuff when older changes weren't even beta tested.

@tcitworld
Copy link
Member

@tcitworld can you shed some light on release plans? It is problematic to write new stuff when older changes weren't even beta tested.

Unfortunately there's been nearly no activity lately, apart from @Kdecherf, so no idea. https://app.wallabag.it is running on master though.

@di72nn
Copy link
Member Author

di72nn commented Mar 12, 2020

I meant the android app release plans.

@di72nn
Copy link
Member Author

di72nn commented Mar 14, 2020

Added a dialog that appears when a url is shared to wallabag app. The dialog allows to tag, favorite and archive an article right after it was added (even offline). Fixes #675.
Also allows to open the added article (but only after it gets synced in background). Fixes #868.

@di72nn di72nn force-pushed the service_reorganization branch 2 times, most recently from c6593d1 to f95fac0 Compare March 16, 2020 15:41
@di72nn
Copy link
Member Author

di72nn commented Mar 16, 2020

Tags in article view (clickable). Fixes #464.
<untagged> entry in tag list to see untagged articles. Fixes #547.
Context menu for article lists (non-bulk only). Fixes #233.

@di72nn
Copy link
Member Author

di72nn commented Mar 31, 2020

Pushed a separate branch with TTS-related refactoring. Not sure how stable it is yet.
TTS is in a separate PR: #945.

@tcitworld
Copy link
Member

@di72nn Please rebase and then we'll get this in.

@di72nn
Copy link
Member Author

di72nn commented Apr 1, 2020

@tcitworld there's a lot of changes in the master already, can you release a beta? It would be great to have a release before merging this.

@tcitworld
Copy link
Member

okay let's try this

@di72nn di72nn force-pushed the service_reorganization branch from f979b46 to c7385e6 Compare April 1, 2020 10:39
@di72nn di72nn mentioned this pull request Apr 1, 2020
5 tasks
@di72nn di72nn force-pushed the service_reorganization branch from b77fbeb to 3cbb437 Compare April 9, 2020 09:58
@di72nn
Copy link
Member Author

di72nn commented Apr 9, 2020

Moved all the commits from #945 here, because it's effectively the same PR now.

@di72nn
Copy link
Member Author

di72nn commented Apr 9, 2020

GPlay Console shows 107 "installs on active devices" for the latest beta, and no new crashes.

@tcitworld I think this PR is ready for another beta.

@di72nn di72nn force-pushed the service_reorganization branch 3 times, most recently from 377e67d to 66afba9 Compare April 10, 2020 11:41
@di72nn di72nn force-pushed the service_reorganization branch from 01b7350 to d195215 Compare April 18, 2020 09:16
@di72nn
Copy link
Member Author

di72nn commented Apr 18, 2020

Updated some minor things. Sorry for all this history-rewriting noise - I figure it's better to fix things right where they were introduced.

@di72nn di72nn mentioned this pull request Apr 19, 2020
@Strubbl
Copy link
Contributor

Strubbl commented Apr 21, 2020

So far the app works as expected in my daily usage. TTS not tested yet.

@di72nn
Copy link
Member Author

di72nn commented May 5, 2020

@tcitworld any news on the review?

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