Add selection feature support

[desaintmartin]

Fixes: #105

Licence: AGPL

Description

• Add the ability for user to select and download a selection of pictures in an album
• Fix image label visibility when mouse is over it at load time
• Also work on acceptance testing (create test helpers, make it work on Travis-ci)

As I am not proficient with the Nextcloud/Gallery codebase/conventions, I'm happy to change whatever is needed (controlers vs views, Files code uses, codestyle, several pull-requests, etc).

Features

• Add selection feature support.
• Fix image label visibility when mouse is over it at load time
• Travis-ci:
  • Add dbus-x11 package needed for Firefox testing
  • Update selenium to v3 for modern Firefox versions support
  • Add access_log after failure
  • Update Codeception version to 2.3

Caveats

• OCA.Files.* objects are used with js files coming from Files app put in js/vendor directory. Maybe it is wiser to adapt it to Gallery, maybe it is fine this way to have convergence
• To be implemented: total selected files count, total selected files size, move files, delete files, album downloads, ...
• Travis has problems with intermitent failures with acceptance tests regarding login (which is still better than no test at all but far from ideal)
• Acceptance tests are painful to write because of the way moveMouseOver is being used (for some reason, I am not able to make it work when an element is set as argument).

Tests

Acceptance tests for :

• Download a selection of pictures
• Download a selection of pictures as anonymous
• Label Visibility issue

Tested on

• [✓ ] Windows/Firefox
• [ ❌] Android 6.1/Chrome: this feature is not intended to work on mobile.

[pixelipo]

I'm a bit skeptical about the amount of new JS (internal and especially vendor) added to Gallery, but I'm not sure what is the official policy of @nextcloud/javascript regarding this.

[oparoz]

First of all, absolutely awesome that you've fixed and added acceptance tests :), but I'd like to ask you to move the fixes to a separate PR in order to not mix too many things in this one. Then you could rebase this PR on top of the fixes.

Now for the PR itself, it may take me a while to get back to you, but I'm hoping the community will help review your work.

[desaintmartin]

Regarding amount of new JS, it really depends on what is the Gallery policy. To be honest, I've brainlessly added what was needed from Files in vendor and wanted to discuss about this with you but with a working feature before discussing. We have 3 possibilities :

• Keep it as is
• Shave the Files files in vendor to keep only relevant code (not good from maintenance perspective)
• Integrate all needed code (OCA.Files notably, and also all other used helpers) into Gallery The fourth one being using the same code between Files and Gallery for a set of features, which would make sense from my point of view but is out of the scope of this PR. As a side note, I've made sure selection in Gallery works the same way as in Files from a code/logic perspective.

About size of code inside of Gallery itself, I am myself surprised of the amount it needed to be implemented, but once again I kept the logic used in Files to do the same thing (and promised myself when I took this issue a few weeks back to finish it whatever it takes. Stupid me).

Acceptance testing : I'll then create another PR this evening to split it up.

[pixelipo]

Aah, in that case, I would suggest using JS addClass/removeClass.

Loading icon has recently been moved to CSS anyway

[desaintmartin]

@pixelipo, I've pushed a new commit to the PR addressing the CSS issue, if you are happy with it I can squash it.

[desaintmartin]

Hello, I've squashed the commit for readability. As a side effect, we need one less file from Files.

Once again, I'm ready to adapt my code for any existing convention/policy in use in Gallery / Nextcloud, see my previous posts for examples.

[MorrisJobke]

@oparoz Please have a look at this.

[desaintmartin]

Hello, Any news on this?

[MorrisJobke]

cc @nextcloud/designers @danxuliu @oparoz for review

[oparoz]

So here are my initial questions:

• Why is there a new js/merged_public.json ? I thought we had this before. Has the file been removed for some reason?
• As you mentioned @desaintmartin , we do not want to link to JS found in the Files app. For the upload part, I re-created only the functions I needed in upload-helper.js, maybe this file should be renamed to files-helper.js and you should add there the methods that you need?

[desaintmartin]

1/ https://github.com/nextcloud/gallery/pull/328/files#diff-40feb0f13355b6f24487bc09704407e0L10 shows that the same merged.json (used for the not-public part) was used even for the public part and a quick search for "merged" tends to confirm that. Maybe different some file was used before but I haven't seen anything like that lately. 2/ Ok, I will extract the needed functions in that file, I'll try to do it soon.

[desaintmartin]

Hello, waiting for https://github.com/nextcloud/gallery/issues/403 to be solved so that tests can pass regarding public view (and hopefully no change to do in my code once it gets solved). I've spent quite a long time hunting intermitent test failures, it should be okay now. Having two different acceptance test systems for local and CI causes some headaches, it may be interesting to change so that both use the same. I've put the necessary Files-related functions into files-helper.js. The FileSummary object still have size and directory related stats that I don't use, but adding support for this in Gallery may be a good next step so I kept it.

I've also changed a few things here and there in the tests and in excluded files, tell me if it fits you.

Tell me if it looks good so that I'll squash the last commit into the main one.

[desaintmartin]

Now that it is all green, do you agree with the latest two commits (and the other ones)? If so, I'll squash them and it'll be good to merge, hopefully.

[codecov[bot]]

Codecov Report

Merging #328 into master will not change coverage. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #328   +/-   ##
=========================================
  Coverage     82.61%   82.61%           
  Complexity      360      360           
=========================================
  Files            38       38           
  Lines          1323     1323           
=========================================
  Hits           1093     1093           
  Misses          230      230

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7c96dc6...3760d5e. Read the comment docs.

[desaintmartin]

Without response, I merged the new commits into the main one. It feels nice to see green everywhere for tests!

[desaintmartin]

Hello, Has this project been temporarily abandoned in favor of the German government project? :)

[desaintmartin]

Hello guys, One year later, is there anybody interested by this feature or should I just drop it?

[juliushaertl]

I'm sorry this has been lying around for that long, I'll look into your changes the next days.

[juliushaertl]

@desaintmartin Sorry, this has somehow slipped under the radar. Could you please rebase your changes to master, since there are some conflicts.

[desaintmartin]

There are a lot of conflicts unfortunately, I'll have a look when I have time.

[sjiampojamarn]

Anything we can do here. This feature is a must; deal breaker.

[nickvergessen]

The gallery app has been replaced by the beautiful new app: Nextcloud Photos - :camera_flash: Your memories under your control

Please checkout if your Pull request is still necessary there, and in case create it there or raise an issue for others to copy the change from here.