pmcaFilesystemServer icon indicating copy to clipboard operation
pmcaFilesystemServer copied to clipboard

API framework and new web UI

Open ov3rk1ll opened this issue 4 years ago • 31 comments

Summay

I have been using this app for a file to get images but it's sometimes hard to quickly find the image you want. So I decided to build a new web interface with thumbnails and some more information. The communication is done via a JSON API to get a list of files and exit data.

The initial list groups the files by day so you can easily jump to a specific date and also let's you filter by file type. pmcaFilesystemServer-better-ui-1

When clicking on a entry, you get more details from EXIF data, a larger preview and also a download button that opens the actual file pmcaFilesystemServer-better-ui-2

ToDo

  • [x] Videos don't have thumbnails yet. If those can't be generated at least serve a static fallback file
  • [x] Reading EXIF from RAW files. This seems to somewhat fail at least on my a6000
  • [x] Improve the EXIF data display in the UI
  • [ ] Test on other cameras. I only have access to an a6000 for testing so I can't validate all those features on other devices

ov3rk1ll avatar Jul 22 '20 10:07 ov3rk1ll

Wow, it seems like your putting a lot of effort into this, nice! Haven't had a chance to look into the code yet, but sounds like a wonderful addition from what you write.

I have just set off to an extensive holiday, trip so I probably won't have time to have a closer look for the next couple of weeks

Once I'm back I can try out on an A6500 and perform a thorough code review.

Ah and just an idea, unit tests would make your features more sustainable. I didn't write any in this project to begin with because I didn't have much time and needed the feature desperately 🙈 Also, I never thought it would be that useful for others. But that's up to you!

schnatterer avatar Jul 23 '20 19:07 schnatterer

Thanks for your feedback.
Just let me know if there is anything you want to change after you had a chance to look that the feature.

Regarding unit tests: I have looked into this a bit but there are some strange limitations when I run them on my camera. I'm unable to push test files and code coverage also returns no data. Nevertheless will I write as many tests as possible for the new API but I might put them in a new pull request to keep the discussion separated.

ov3rk1ll avatar Jul 27 '20 11:07 ov3rk1ll

I have now pushed all the unit tests I could think of for the new API. There are no tests for the javascript I'm not really sure how that would work with the current setup.

There are some limitations when I run those tests on the actual camera:

  • Must be run via "connectedCheck" or "connectedAndroidTest" command
  • Pushing test files (via adb push) fails so we have to rely on existing files on the device.
    The test expect at least one image, video and raw file to work.
  • Code coverage doesn't return any data so tests would need to run in a emulator for that

The travis build now fails because of the "connectedCheck" command in the build script

ov3rk1ll avatar Jul 30 '20 07:07 ov3rk1ll

Sounds great! Regarding Travis two things come to mind:

  • Do the test run on an emulator? Then we could launch one during the build
  • Could we make them "proper" unit tests using robolectric. The question here is: do we really need a device for the unit tests?

schnatterer avatar Jul 30 '20 08:07 schnatterer

I have no updated the project with some test files that get pushed to the emulator before running the tests.
I tried adding a emulator to the travis file but that seems to timeout/fail for now.

  • Could we make them "proper" unit tests using robolectric. The question here is: do we really need a device for the unit tests?

I don't have any experience with robolectric but the current unit tests do run in the emulator.

Given how "special" the Android OS on those cameras is, I'd suggest to run the tests on a actual hardware as well. At lease for release builds.

ov3rk1ll avatar Aug 03 '20 16:08 ov3rk1ll

@ov3rk1ll I changed the unit tests to run without emulator via robolectric. This increases the CI build significantly (3 min instead o 12). Also the tests should run faster locally. Please have a look if everything still runs as intended.

Next stop is manual test on my device and code review. Do you still consider the state of this PR as WIP? Or is it ready to merge from your side?

schnatterer avatar Aug 11 '20 20:08 schnatterer

@ov3rk1ll I had a glance at the UI from a user's perspective. It's awesome! Such an improvement to the minimalistic first version in both design, usability and features! It also fixes #2 👍 Thanks for your efforts!

The only addition, I would love to see is a visualization for the loading process while the thumbnails are fetched. On my camera the loading takes up to 10s with > 1000 jpgs (which is ok, considering it's a camera). However, during the wait I was starting to wonder whether there is any progress. This could be resolved by a loading spinner, for example.

My code review is still pending.

schnatterer avatar Aug 13 '20 20:08 schnatterer

I have added a progress/loading indicator for both the list and zip file.

I'd say that feature-wise, this PR is done so I'd not consider this WIP anymore.

ov3rk1ll avatar Aug 14 '20 13:08 ov3rk1ll

Just chiming in to say that I can't wait to try this out on both my A6000 and A6500 👍

nrk avatar Aug 15 '20 11:08 nrk

@ov3rk1ll I moved all JS dependencies to a package.json. They are now resolved via npm. This provides a number of advantages, for vuln scaning, updating, etc.

With npm in place, we're now a huge step closer to adding JS unit tests, e.g. via jest, or minify web resources, etc. If you're still interested in writing tests for JS, I could boostrap some tests later. Do you know if icomoon can be resolved via npm as well? I worked with fontawesome before, where this is possible.

One downside, the theme is now no longer black :-/ Please have a look at this and check if everything still works. I'm not sure if I got the version and each dependency right.

schnatterer avatar Aug 16 '20 19:08 schnatterer

I have changed the bootstrap import back to the darkly file for the theme. The rest of the dependencies look okay to me.

I don't think icomoon would work via npm. It's a tool where you provide a set of SVG files to build a font from.

~~It might be possible to do that step at build-time (eg. https://www.npmjs.com/package/icon-font-generator) but I have not looked into something like that yet.~~ This is possible and I have added this in the last commit. Please take a look if the implementation in the build file is okay like that. I have never worked with node inside gradle so I'm not sure if that is the best way to do this.

I have attached the project for https://icomoon.io/app/ . Make sure to use the JSON inside the ZIP to import the project (Gitlab will not let me upload a JSON directly 🤷‍♂️)

ov3rk1ll avatar Aug 17 '20 09:08 ov3rk1ll

Java review done. Last thing missing JavaScript review. Hopefully I can get it done next week or weekend.

In Java, I basically refactored with maintainability in mind, from what I learned about clean code during the last years. Smaller functions, exception handling, less conversion between collections, etc.

We probably could refactor further,

  • into smaller classes instead of a monolithic HTTPServer class
  • use more constants for URLs, params, JSONs. But, I spent too much time already, have to move on 😬

I changed a lot. It was really helpful to have at least the basic unit tests. Thanks very much for taking your time to write them! :-)

Please review my changes thoroughly and test if everything is still as it should be. On my A6500 the basic use cases still seem to work.

Some questions that arose during review:

Urls

  • Why is /thumbnail.do not a sub resource of /api like the others?
  • Why is /assets a plural and the other URLs singular, ending in .do?
  • Why .do and not plurals, e.g. /thumbnails? It might be subjective but for me those feel much more common.

Functional

  • When trying to fallback to JPG - are files always upper case on sony cams? Or should we also try lower .jpg to be more defensive?
  • Did you try creating video thumbnails, e.g. like so?

Others

  • Do we still need this? response.addHeader("Access-Control-Allow-Origin" "*");
  • Is there a reason why you implemented decodeParameters() instead of using IHTTPSession.getQueryParameterString()? If so please document. If not, please get rid of decodeParameters(). No code is better than no code.
  • While modifying the code I wished the unit tests would assert the results more thoroughly. Even though the tests are successful I'm not convinced didn't break anything during refactor E.g.
    • Is the metadata read properly?
    • Assert the expected sizes of the test image and thumbnails not just > 0.
      (e.g. the thumbnail extracted from exif tag might > 0 but the wrong number of bytes "cut" from the source file)
  • Did you use fileExt.equalsIgnoreCase("ARW") instead of FilesystemScanner.rawFormats.contains("." + fileExt.toLowerCase() for a specific reason? I changed that to be consistent with the other file extension checks (video and jpg)
  • npm install results in an execution about ttf2woff2 but does not fail (see travis CI logs). This seems to be related with using Node 12 (which is the latest LTS). Tried to solve it by forcing ttf2woff2 to the newest major version 3.0.0 as suggested here without luck. Any ideas?
  • Can you read the log file on your device? This probably has nothing to do with this issue, but on my A6500, it always returns 404. If it doesn't work for you as well, this is probably worth a new issue.

schnatterer avatar Aug 23 '20 13:08 schnatterer

Getting started with the JavaScript part, I introduced webpack to get JavaScript modules with clearer scoping and dependencies, which also builds a single js dependency for less HTTP requests at runtime. Using modules and scopes now finally allows for unit testing our JS code.

There were some global variables that made the whole process challenging for me. I'm not sure if I converted all globals properly. For example, meta here. Maybe you could check if everything still works? At least there now is the option of finding and fixing those issues in a more efficient way using unit tests 😉

Also, jquery is horrible for getting tests bootstrapped, but my proof of concept should give an idea how it could work.

schnatterer avatar Aug 29 '20 13:08 schnatterer

Hi @ov3rk1ll , Did I ask too many questions 🙈 ?

Are you planning to continue working on this? If not, could I ask you to answer my question on the Java code and maybe have a quick review of my changes to JavaScript? I can then merge the PR and decide how to go on from there myself.

Either way, thanks for your efforts up to this point!

schnatterer avatar Oct 11 '20 11:10 schnatterer

@schnatterer I sadly had absolutely no time to work on this over the last months so it's definitely not the number of questions.

I'll try to get back to you with the information in the next few days but I can't really say how much time I'll have to work on this.

ov3rk1ll avatar Oct 15 '20 19:10 ov3rk1ll

@ov3rk1ll thanks for getting back to me. I'm in no hurry, if you're still motivated to continue, take your time. I just want to make sure our work gets merged eventually.

schnatterer avatar Oct 15 '20 19:10 schnatterer

Hey Guys

I just tried to install it on my Sony Alpha 7II - i built it in Android Studio on Mac OS and tried to install on camera, but i got many errors. At the same time, i haven't any problem when installing from the app list.

for example,

Traceback (most recent call last): File "pmca-gui.py", line 78, in do File "pmca\commands\usb.py", line 300, in installCommand File "pmca\commands\usb.py", line 73, in installApp Exception: Communication error 100: Error completed`

I understand that you are planning to do a little optimisation, but i would like to have it installed the way it is. Probably, you need help with testing?

*tried to build it on windows - the same

oresamp avatar Oct 28 '22 10:10 oresamp

Hi @Oresamp, I built it back then and running it on my 6500 ever since. Building it with a contemporary version of the Android tools might be difficult now 😐 I didn't have a lot of free time on my hands since, and probably won't have too soon. I might be able to upload the apk as a preview, if I can dump it off my camera.

schnatterer avatar Nov 04 '22 21:11 schnatterer

FWIW I dumped the apk of this PR that I've been using on my camera for years and uploaded it as a pre-release. So, for the time being - if you're happy running a pre-release you can find the apk here.

Feedback welcome, but I cant promise anything. CC @oresamp @nrk

schnatterer avatar Feb 10 '23 13:02 schnatterer

@schnatterer I finally tried the apk you attached. I have the same error as before in October.

Traceback (most recent call last): File "pmca-gui.py", line 78, in do File "pmca\commands\usb.py", line 300, in installCommand File "pmca\commands\usb.py", line 73, in installApp Exception: Communication error 100: Error completed`

this happens both with this apk and the previous one, which was part of Sony-PMCA-RE. I suggest it got the same apk now.

oresamp avatar Apr 01 '23 18:04 oresamp

@oresamp have you tried installing it via adb?

  • Install the open memories tweak app and start it on your camera.
  • Enable wifi
  • Enable adb
  • on your computer, open a console and type
adb tcpip 5555
adb connect <ip address as shown on camera>:5555
adb push your.apk
# If this fails you might want to try the following
adb install -t your.apk

For adb install see @dan-r's post.

schnatterer avatar May 31 '23 09:05 schnatterer

@schnatterer unfortunately for me is : adb: error: failed to copy 'info.schnatterer.pmcaFilesystemServer-1.apk' to 'device': remote Read-only file system

oresamp avatar Jun 16 '23 14:06 oresamp

@oresamp What about adb install?

schnatterer avatar Jun 16 '23 15:06 schnatterer

@schnatterer Tried also. Push, install, reboot.. All what I can find.

I'm thinking about API Version Android Debug Bridge version 1.0.41 Version 34.0.3-10161052 Installed as /opt/homebrew/bin/adb Running on Darwin 22.5.0 (arm64)

but it's interesting, that some people cannot also install from Sony-PMCA-RE with the same error:

oresamp avatar Jun 17 '23 10:06 oresamp

Sony A6000 PAL: I installed the stable 0.2.0 and noticed there was no GUI, so I decided to try the 0.3.0-pre.

1: Sony-PMCA-RE

I also ran into this error:

File "pmca-gui.py", line 78, in do
File "pmca\commands\usb.py", line 300, in installCommand
File "pmca\commands\usb.py", line 73, in installApp
Exception: Communication error 100: Error completed

( The pmca\ file path suggests that some uncaught error in the installation logic was eventually caught by the PMCA-RE usb driver. )

2: adb (via OpenMemories-Tweak, over wifi) With adb install, I saw an error about the INSTALL_FAILED_TEST_ONLY. I tried adb push + shell install, but I saw an error saying something about INSUFFICIENT_SPACE.

I was looking around for fixes online, so the camera turned off a couple of times meanwhile. I tried the adb install (not push + install) again and it threw an error saying that I have the app already installed (I'm not sure if it meant the stable 0.2.0 I already had or if somehow the 0.3.0-pre made it through) So I deleted the version I had (no idea which one), and did the adb install again (not push + install) and the app was installed successfully.

I can use the GUI pretty flawlessly.

What could have worked: Rebooting the device (or letting it hibernate and waking it up) Installing the old version, letting it fail due to the old version, uninstalling the old version

Hope this helps.

JetErorr avatar Jul 13 '23 18:07 JetErorr

I wasn't able to install with PMCA-RE either, and when I tried installing with ADB I got INSTALL_FAILED_TEST_ONLY. I was able to successfully install with

adb install -t NAME.apk

Looks like something wrong in the manifest?

https://stackoverflow.com/questions/25274296/adb-install-fails-with-install-failed-test-only

dan-r avatar Oct 25 '23 22:10 dan-r

Thanks, @dan-r , that sounds plausible. I uploaded a dev version which I installed on my camera using Android Studio. So it might well have the test attribute 💡 I'll update my instructions posted above.

schnatterer avatar Oct 31 '23 16:10 schnatterer

adb install -t NAME.apk

works! Thank you

oresamp avatar Nov 01 '23 21:11 oresamp

Thanks for your app, I was able to install on my Nex 5r with -t parameter in adb. If you alrealdy have an older version of app you have to remove it before install with -t command.

leonardcoutinho avatar Mar 03 '24 23:03 leonardcoutinho

I wasn't able to install with PMCA-RE either, and when I tried installing with ADB I got INSTALL_FAILED_TEST_ONLY. I was able to successfully install with

adb install -t NAME.apk

Looks like something wrong in the manifest?

https://stackoverflow.com/questions/25274296/adb-install-fails-with-install-failed-test-only

I have tried by unpacking apk, removing test attributes in AndroidManifest.xml file and packing it again. new apk still gives same error in pcma gui v0.17.

seems video is not possible to run in "v0.3.0-pre" which was possible to run in "v0.2.0"

kishorpatel82 avatar Apr 05 '24 15:04 kishorpatel82