#21 local repo

[ramack]

Some cleanup still needed, but let's do it in master.

finally this shall close #21 by introducing another level of repository and splitting out all REST related stuff

open

• [x] add local database
• [x] add local storage of images
  • [x] for now don't expose the downloaded images into a shared folder, but store only in cache
  • [ ] add two download size option: -> #232
• [ ] deliver high-resolution variants in a second loop (if needed) -> #232
• [ ] delete images that have been remove on the server from the local database and cache (this we need even with having exposing in #222 as separate feature, as the database needs to be updated. -> #233
• [x] multi account support
• [ ] use local image storage (ImageRepository) also for album thumbnails (filling and reading), fix the UserAgent-string in the HTTP header
• [x] limit number of parallel downloads
• [x] check how we want to handle "exposing" to other apps / shared storage -> see #222
• [x] adjust album fetching to the same approach as image fetching
• [x] remove flickering, by fixing BindingRecyclerViewAdapter
• [x] speed up download, it seems, that the downloading threads are blocked by a DB access
• [x] adjust the upload code to use the ImageRepository instead of directly REST. -> #231
• [x] update unittests

so far fixes #144

[coolo]

I realize that most other improvements are blocked by this, so I gave it a little test.

And it's crashing on larger galleries:

E/ImageRepository: download failed
    java.net.SocketException: socket failed: EMFILE (Too many open files)
        at java.net.Socket.createImpl(Socket.java:487)
        at java.net.Socket.getImpl(Socket.java:547)
        at java.net.Socket.setSoTimeout(Socket.java:1175)
        at okhttp3.internal.connection.RealConnection.connectSocket(RealConnection.kt:266)
        at okhttp3.internal.connection.RealConnection.connect(RealConnection.kt:176)
        at okhttp3.internal.connection.ExchangeFinder.findConnection(ExchangeFinder.kt:236)
        at okhttp3.internal.connection.ExchangeFinder.findHealthyConnection(ExchangeFinder.kt:109)
        at okhttp3.internal.connection.ExchangeFinder.find(ExchangeFinder.kt:77)
        at okhttp3.internal.connection.Transmitter.newExchange$okhttp(Transmitter.kt:162)
        at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.kt:35)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:112)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:87)
        at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.kt:82)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:112)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:87)
        at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.kt:84)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:112)
        at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.kt:71)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:112)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:87)
        at org.piwigo.io.WebServiceFactory.lambda$buildOkHttpClient$0(WebServiceFactory.java:89)
        at org.piwigo.io.-$$Lambda$WebServiceFactory$VuX3yMEucfqZcmnX6QuQvz4tuOM.intercept(Unknown Source:6)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:112)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:87)
        at okhttp3.logging.HttpLoggingInterceptor.intercept(HttpLoggingInterceptor.kt:215)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:112)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:87)
        at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.kt:184)
        at okhttp3.RealCall.execute(RealCall.kt:66)
        at retrofit2.OkHttpCall.execute(OkHttpCall.java:188)
        at retrofit2.adapter.rxjava2.CallExecuteObservable.subscribeActual(CallExecuteObservable.java:45)
        at io.reactivex.Observable.subscribe(Observable.java:12267)
        at io.reactivex.internal.operators.observable.ObservableSubscribeOn$SubscribeTask.run(ObservableSubscribeOn.java:96)
        at io.reactivex.Scheduler$DisposeTask.run(Scheduler.java:578)
        at io.reactivex.internal.schedulers.ScheduledRunnable.run(ScheduledRunnable.java:66)
        at io.reactivex.internal.schedulers.ScheduledRunnable.call(ScheduledRunnable.java:57)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
        at java.lang.Thread.run(Thread.java:764)
W/zygote64: ashmem_create_region failed for 'indirect ref table': Too many open files
W/zygote64: ashmem_create_region failed for 'indirect ref table': Too many open files

But I'm not so sure if this isn't a followup on this error (I now uninstalled the app to hopefully get a refresh on the permissions):

E/ImageRepository: download failed
    org.piwigo.helper.PermissionDeniedException android.permission.WRITE_EXTERNAL_STORAGE
        at org.piwigo.data.repository.ImageRepository.lambda$downloadURL$3$ImageRepository(ImageRepository.java:302)
        at org.piwigo.data.repository.-$$Lambda$ImageRepository$deZY6wgPSc31_FgFYOM8B0D8GdQ.apply(Unknown Source:17)

Even with this error, the app works though.

[coolo]

No, not a followup error. So the permission grant is independent on this one.

[coolo]

But I love the user experience upgrade. Finally the images are loaded in a transparent way and I love the 'expose photos' option. What is strange though (already on master) that the tumbnails are loaded in xxlarge if I want large resolution pictures on my phone. This only works over wifi. I think from a user perspective this setting shouldn't affect thumbnails but only image views (i.e. fetch during sliding) - and as such only expose those.

[coolo]

I could increase the stability by avoiding multiple download services per account:

--- a/app/src/main/java/org/piwigo/data/repository/ImageRepository.java
+++ b/app/src/main/java/org/piwigo/data/repository/ImageRepository.java
@@ -82,6 +82,7 @@ public class ImageRepository implements Observer<Account> {
     private final Context mContext;

     private WebServiceFactory mWebServiceFactory;
+    private Map<Account, org.piwigo.io.DownloadService> downloadServiceMap;

     private Object dbAccountLock = new Object();
     private CacheDatabase mCache;
@@ -247,7 +248,15 @@ public class ImageRepository implements Observer<Account> {
             return Observable.empty();
         }else {

-            org.piwigo.io.DownloadService downloadService = mWebServiceFactory.downloaderForAccount(a, addHeaders);
+
+            org.piwigo.io.DownloadService downloadService = null;
+            if (a != null) {
+                downloadService = downloadServiceMap.get(a);
+            }
+            if (downloadService == null) {
+                downloadService = mWebServiceFactory.downloaderForAccount(a, addHeaders);
+                downloadServiceMap.put(a, downloadService);
+            }

             Observable<String> result = downloadService.downloadFileAtUrl(url)
                     .subscribeOn(Schedulers.io())

(this comes from someone who didn't code java since '98, sorry)

But I noticed the gallery order isn't preserved, it appears pretty random, but it's stable.

[ramack]

welcome here @coolo the missing dialog to ask for storage permission is known. I am also not yet sure where the best location would be to store the downloaded images. If not exported probably best in cache directory - which is (as far as I know) not required to have the storage permissions in some versions of android, so it is not so obvious...

Album thumbnails are also "only" photos in the gallery, so loading shall be adapted to use the ImageRepository in some way (probably via image Id based loading), then the download size will be the same. Also I want to have adaptive loading such that thumbnails are loaded fast ans stored. If a higher resolution is necessary (e. g. in full screen mode) that low res image should be shown first and then after a higher resolution was loaded the image is redrawn.

Your proposal to reduce the amount of DownloadServices seems reasonable, but I guess we should even reduce it to a single one for the active account.

Your comments are very helpful, thanks. I adjusted my top comment here with some todos!

[coolo]

I wouldn't make it that complicated - at least for now. If I may make a proposal: Download everything into cache and make exposing an explit action on galleries later I can trigger on wifi. You run into danger to want too much at once.

Handling the download and caching in a robust way would already be a big win.

[coolo]

The more I think about it, the more I'm convinced that this is the right approach (sorry, I'm easily convinced by myself :)

On regular usage of the app just download into cache - and IMO don't worry about deleting from it, android offers enough ways to clean up stale caches.

And have an explit action to sync galleries into local storage. This explict action can then make sure online pictures disappear and changed pictures are refreshed, etc. IMO a list of galleries to sync similiar as https://www.dev2qa.com/android-custom-listview-with-checkbox-example/ would be a good opportunity to ask for permissions as well.

While I love the ability to download full folders, I find it a little hard to use the way you set it up in your branch.

[ramack]

You run into danger to want too much at once. You know already a lot about my weaknesses :-) But yes, you are right.

The more I think about it, the more I'm convinced that this is the right approach (sorry, I'm easily convinced by myself :) Being convinced is a good basis for a fruitful discussion!

On regular usage of the app just download into cache - and IMO don't worry about deleting from it, android offers enough ways to clean up stale caches.

Yes and no. I moved the exposing into #222 (If you feel it should be split even more into exposing and offline availability without the danger of a deleted cache feel free to create a new ticket for one or the other.) The deletion is necessary at least in the database, as I want to have the mapping of images to categories and to have a possibility to still show images even without internet and then we have to get rid of the entries if a picture was deleted on the server, otherwise it would still be shown. And if we delete the entry from the database it should be simple to also remove the cached image file.

And have an explit action to sync galleries into local storage. This explict action can then make sure online pictures disappear and changed pictures are refreshed, etc. IMO a list of galleries to sync similiar as https://www.dev2qa.com/android-custom-listview-with-checkbox-example/ would be a good opportunity to ask for permissions as well.

The list should be a tree, but yes, we could have such a thing in the settings with #222

While I love the ability to download full folders, I find it a little hard to use the way you set it up in your branch.

This was not yet cleanly designed. I just stumbled over it when looking where to store the images and though it would be pretty cool to have the images available as usual directory in the pictures, as many other apps do also while on the other side I also would like to support an approach where maybe other, less trustworthy apps have access to the public picture store and sensitive data is in the piwigo gallery, so I added the setting to expose...

[coolo]

I tried rebasing this to see where I can help, but it's still quite bugged.

I had to apply the following to get it moving a little further:

index 24f31d1..7e3d8ef 100644
--- a/app/src/main/java/org/piwigo/ui/main/MainActivity.java
+++ b/app/src/main/java/org/piwigo/ui/main/MainActivity.java
@@ -94,7 +94,6 @@ import dagger.android.AndroidInjector;
 import dagger.android.DispatchingAndroidInjector;
 import dagger.android.HasAndroidInjector;
 import io.reactivex.disposables.Disposable;
-import rx.Subscriber;

 public class MainActivity extends BaseActivity implements HasAndroidInjector {
     private static final String TAG = MainActivity.class.getName();
@@ -215,7 +214,8 @@ public class MainActivity extends BaseActivity implements HasAndroidInjector {
                         Log.i(TAG, "Login succeeded: " + loginResponse.pwgId + " token: " + loginResponse.statusResponse.result.pwgToken);
                         userManager.setCookie(account, loginResponse.pwgId);
                         userManager.setToken(account, loginResponse.statusResponse.result.pwgToken);
-                        userManager.setChunkSize(account, loginResponse.statusResponse.result.uploadFormChunkSize);
+                        if (loginResponse.statusResponse.result.uploadFormChunkSize != null)
+                            userManager.setChunkSize(account, loginResponse.statusResponse.result.uploadFormChunkSize);
                     }
                 });
                 initStartFragment(viewModel);
diff --git a/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerDialogFragment.java b/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerDialogFragment.java
index b89a5dc..59c9089 100644
--- a/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerDialogFragment.java
+++ b/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerDialogFragment.java
@@ -8,10 +8,12 @@ import android.view.ViewGroup;
 import androidx.fragment.app.DialogFragment;
 import androidx.viewpager.widget.ViewPager;

+import com.google.errorprone.annotations.Var;
 import com.squareup.picasso.Picasso;

 import org.piwigo.R;
 import org.piwigo.data.model.Image;
+import org.piwigo.data.model.VariantWithImage;

 import java.util.ArrayList;

@@ -21,7 +23,7 @@ import dagger.android.support.AndroidSupportInjection;

 public class PhotoViewerDialogFragment extends DialogFragment
 {
-    private ArrayList<Image> images;
+    private ArrayList<VariantWithImage> images;
     private ViewPager viewPager;
     private PhotoViewerPagerAdapter pagerAdapter;
     private int selectedPosition = 0;
@@ -39,7 +41,7 @@ public class PhotoViewerDialogFragment extends DialogFragment
     {
         View v = inflater.inflate(R.layout.fragment_fullscreen_images, container, false);
         viewPager = v.findViewById(R.id.viewpager);
-        images = (ArrayList<Image>) getArguments().getSerializable("images");
+        images = (ArrayList<VariantWithImage>) getArguments().getSerializable("images");
         selectedPosition = getArguments().getInt("position");
         pagerAdapter = new PhotoViewerPagerAdapter(getContext(), images);

diff --git a/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerPagerAdapter.java b/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerPagerAdapter.java
index 14efb4b..409255c 100644
--- a/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerPagerAdapter.java
+++ b/app/src/main/java/org/piwigo/ui/photoviewer/PhotoViewerPagerAdapter.java
@@ -11,17 +11,18 @@ import com.squareup.picasso.Picasso;

 import org.piwigo.R;
 import org.piwigo.data.model.Image;
+import org.piwigo.data.model.VariantWithImage;

 import java.util.List;

 public class PhotoViewerPagerAdapter extends PagerAdapter {

     private Context context;
-    private List<Image> images;
+    private List<VariantWithImage> images;
     private LayoutInflater inflater;
     private Picasso picasso;

-    public PhotoViewerPagerAdapter(Context context, List<Image> images)
+    public PhotoViewerPagerAdapter(Context context, List<VariantWithImage> images)
     {
         this.context = context;
         this.images = images;
@@ -34,10 +35,10 @@ public class PhotoViewerPagerAdapter extends PagerAdapter {
         inflater = (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
         View view = inflater.inflate(R.layout.item_fullscreen_image, container, false);
         TouchImageView imageViewPreview = view.findViewById(R.id.imgDisplay);
-        Image image = images.get(position);
+        VariantWithImage image = images.get(position);

         // TODO: trigger URL updates to get needed resolution and handle LiveData updates
-        picasso.load(image.elementUrl).noFade().into(imageViewPreview);
+        picasso.load(image.image.elementUrl).noFade().into(imageViewPreview);
         container.addView(view);
         return view;
     }

[coolo]

I'd really love if you could polish to end in master. I'd love to rebase my login handling fixes on top of it and look out for other bugs, but master as is is uninteresting to work with :)

[ramack]

ok, so let's the show begin... It is not clean, but still a step forward, so I merge that and we can cleanup in master. Thanks for your patience.

[coolo]

well, I would have preferred some squashing away the WIPs. Git history is a value that's quite benefital to new contributors. But now it's too late - and I guess I should have expressed my expectations before :)

So, thanks for merging - I'll go ahead and rebase my login changes.

[ramack]

for sure you are fully right, I would also have preferred to squash those commits but I also wanted to keep the commits from you, so I could not just squash it in the github frontend. And for checking how I can do it manually I was too lazy (shame on me). On the other side I also don't think that it is a big issue - I am more relaxed than those who fight for a perfectly linear looking history. At least I did not yet read the convincing argument so maybe I have to learn the lesson by experiencing it.

[coolo]

This is not about linear history, this is about git blame showing 'WIP' as reasoning.

[coolo]

One resource linked in open.qa's contributing guide is https://chris.beams.io/posts/git-commit/ and it does make some good points and guidelines. All to be taken with a grain of salt of course