cuba-platform / guides

CUBA platform guides
Apache License 2.0
7 stars 7 forks source link

Guide: working with images #55

Closed mariodavid closed 5 years ago

mariodavid commented 5 years ago

This guides replaces this Cookbook section:

https://doc.cuba-platform.com/manual-7.0/images_recipe.html

In a CUBA application it is possible to interact with images in various ways. In this guide, examples will be shown on how to upload and display images in the application. It will also be shown how to attach those images to entities and providing the user the ability to download the image files from the application.

knstvk commented 5 years ago
  1. Please put callouts in comments, then they won't break the code when copy-pasted:

    @Subscribe
    protected void onInit(InitEvent event) {
        vetsTable.addGeneratedColumn( // (1)
                "image",
                this::renderAvatarImageComponent
        );
    }
  2. VetPreviewComponentFactory and XrayPreviewComponentFactory need some justification like if you need such component in several screens. Just not to scary newbies.

  3. In "Upload X-Ray Images" section, you use MANUAL mode for upload field, but in fact it results in the same as for IMMEDIATE mode: the file is uploaded to the file storage and FileDescriptor is saved. Moreover, as you do DataContext.commit(), all changed data are saved at this moment, which may confuse users if they click "Cancel" afterwards

    fileUploadingAPI.putFileIntoStorage(upload.getFileId(), imageFile); (2)
    
    dataContext.merge(imageFile); (3)
    xRayImagesDc.getMutableItems().add(imageFile);
    dataContext.commit();

    Unfortunately, the is no simple way to defer saving of the file to the moment when the whole screen is saved on "OK", because we need to display the loaded image in the table, and the file must be in the storage for that. I propose the solution with saving the file and then removing it on "Cancel", see https://github.com/cuba-guides/cuba-petclinic-working-with-images/pull/1

mariodavid commented 5 years ago

hi @knstvk ,

thanks for the feedback.

I'll comment on it one by one.

  1. You are right. This is mentioned layout problem in this issue: https://github.com/cuba-platform/guides/issues/40 that has not been fixed yet. According to Asciidoc I think everything is used correctly. I'm not sure on how to do it differently in the adoc file. It is a layout issue. In fact I used comments in the sources as you can see here: https://github.com/cuba-platform/guides/pull/55/files#diff-422ee6b963fdda8a023e7c29ce7e6668R167

  2. ok, makes sense. Will write a better explanation to understand the reasoning behind it. Mainly it was to not have too long listings and to keep a proper separation of concerns. Reuse was for me in this case only a nice side effect.

  3. I struggled with this one actually (and not for the first time, more like the majority of the times I use the upload component). In the beginning I tried it with intermediate, but then I had the problem that this line FileDescriptor imageFile = upload.getFileDescriptor(); resulted in imageFile being null. Then i tried MANUAL mode and after some fiddling it worked. But right, it has the downside of stale files (even stated in the guide text). Additionally then I thought it would be good to have an example of both modes, therefore I left it like that. But if you have a better idea on how to implement that (and it seems you have, since you created a PR), lets go with that one.

knstvk commented 5 years ago

OK!

if you have a better idea on how to implement that

It's actually not very elegant, but an alternative would be just don't care about orphan files and descriptors in case of cancelling the edit. Actually, the files will become orphans anyway when user removes images from the visit. Maybe we should simplify things and don't remove new files on cancel. Please decide yourself.

mariodavid commented 5 years ago

I added this section as a callout regarding the factory justification:

Bildschirmfoto 2019-08-17 um 11 57 57
mariodavid commented 5 years ago

I've merged the PR, looks much better. I also added a little section to the guide mentioning the clean up stuff, but reference to the example file for more information.