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

Card Cover Images #5035

Merged
merged 2 commits into from
Oct 17, 2023
Merged

Card Cover Images #5035

merged 2 commits into from
Oct 17, 2023

Conversation

jszeibert
Copy link
Contributor

Summary

This is my simple implementation of card cover images, inspired by the CoverImage feature of deck Android app:

I placed the option alongside compact mode and it enabled/disables the option globaly (even for the upcoming cards view):
CardCover-option

The normal board view displays up to 3 images sorted by id (same as in the AttachmentList) to get the latest. This is the same with the Android app.
CardCover-boardView

In compact mode I'm deviating from the android app because of a different column width. Adding one cover immage to the left of the card (like proposed in #2858) reduces space for the title dramatically.
CardCover-compactView

I opted to allow all mime-types that generate a preview to be displayed as card covers. This may include mp3 files with album covers and textfiles. With textfiles this could result in a seemingly blank preview image, as they are centered and if there are to few lines they might be cropped.

Last mentionable thing about my implementation would be that the core preview provider behaves a bit strange providing much larger than needed images when cropping is enabled. Therefore I use the option a=1 to not crop the image and let css handle the overflow.

Finaly, this is the mobile view:
CardCover-mobileView

TODO

  • translations

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

@jszeibert jszeibert changed the title introduce Card Covers Card Cover Images Aug 18, 2023
@juliusknorr
Copy link
Member

Thanks for your pull request, this looks very nice already. I'll try to get to a full review during the week.

For the UI, let me already pull in @nextcloud/designers for some review on the screenshots.

Just a quick note, could you use rebase instead of merge for updating the branch to the latest main branch for a cleaner history? :)

@juliusknorr
Copy link
Member

Translations do not need to be done in the pull request as long as all code parts use the proper t() or n() functions. Once merged the new strings will be added to transifex and be handled by translators automatically.

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments inline, generally this seems very nice also from a code perspective, with the performance flaw that we'd still need to figure out. I'll think a bit about that.

One small bug found during testing:

  • When uploading an image attachment to a card the CardCover component loads but no image is shown

src/components/cards/CardCover.vue Show resolved Hide resolved
cardId: {
immediate: true,
handler() {
this.fetchAttachments(this.cardId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite bad for performance as it will issue one request per card.

Maybe we need to extend the existing card listing request to return the attachments but might also need some care to not run into performance regressions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly for reference, I did a quick patch to list the attachments with the initial request but that has quite some impact due to the required file system setup.

Patch (probably not the way to go)
~/repos/nextcloud/server/apps-extra/deck enh/CardCover✦ ➜ git diff
diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php
index 45559b87..deaed044 100644
--- a/lib/Service/CardService.php
+++ b/lib/Service/CardService.php
@@ -126,6 +126,7 @@ class CardService {
                        $this->cardMapper->mapOwner($card);
 
                        $card->setAttachmentCount($this->attachmentService->count($cardId));
+                       $card->setAttachments($this->attachmentService->findAll($cardId));
 
                        // TODO We should find a better way just to get the comment count so we can save 1-3 quer
ies per card here
                        $countComments = $this->commentsManager->getNumberOfCommentsForObject('deckCard', (string
)$card->getId());
diff --git a/lib/Service/StackService.php b/lib/Service/StackService.php
index ce3e1195..9eaa97eb 100644
--- a/lib/Service/StackService.php
+++ b/lib/Service/StackService.php
@@ -129,6 +129,7 @@ class StackService {
                                $assignedUsers = $this->assignedUsersMapper->findAll($card->getId());
                                $card->setAssignedUsers($assignedUsers);
                                $card->setAttachmentCount($this->attachmentService->count($card->getId()));
+                               $card->setAttachments($this->attachmentService->findAll($card->getId()));
 
                                return new CardDetails($card);
                        },
(END)

Blackfire profile for comparison https://blackfire.io/profiles/compare/b491c795-67e9-444a-ba75-ca5cbc866436/graph

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be ok if we just do it as a separate request then as card cover images would not be default, so users could opt in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still haven't come up with a good idea for that.

One option might be to approach this similar to the attachment count:

public function clearAttachmentCount(int $cardId): void {
In that we try to always keep the count cache up to date if something changes with the shares, but for the attachment list with metadata this would be quite tough to get properly done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a small commit to only request attachments if there are any, which is information we already have around.

Let me merge this already then since it is opt in and we can follow up on backend side optimization of the requests.

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice design wise @jszeibert. I don't have comments to add to this.

As an additional feature, it would be nice to be able to select one specific image as a card cover, but something for the future maybe.

@jszeibert
Copy link
Contributor Author

Thanks for your pull request, this looks very nice already. I'll try to get to a full review during the week.

For the UI, let me already pull in @nextcloud/designers for some review on the screenshots.

Just a quick note, could you use rebase instead of merge for updating the branch to the latest main branch for a cleaner history? :)

Sorry for that. I used the github feature to sync main. Would you like me to rebase and force push to correct this?

@juliusknorr
Copy link
Member

juliusknorr commented Aug 22, 2023

Sorry for that. I used the github feature to sync main. Would you like me to rebase and force push to correct this?

No worries, yes that would be appreciated.

Github also has this function nowadays (if the branch is outdated) but well hidden and no option to make this the default per repository unfortunately:

Screenshot 2023-08-22 at 12 49 33

@jszeibert
Copy link
Contributor Author

jszeibert commented Aug 31, 2023

One small bug found during testing:

  • When uploading an image attachment to a card the CardCover component loads but no image is shown

Yes, that is a strange one, ... either the thumbnail in the Attachment list or the Card Cover isn't rendered on upload.
Somehow with my previous Test this happened only to the thumbnails which I didn't notice.

The error on nextcloud server 27.0.2.1 is:

...
   "url":"/core/preview?fileId=179&x=64&y=64",
   "message":"Could not create folder \"/appdata_oc491gyqkd6i/preview/8/f/5/3/2/9/5/179\"",
   "userAgent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:109.0) Gecko/20100101 Firefox/116.0",
   "version":"27.0.2.1",
   "exception":{
      "Exception":"OCP\\Files\\NotPermittedException",
      "Message":"Could not create folder \"/appdata_oc491gyqkd6i/preview/8/f/5/3/2/9/5/179\"",
      "Code":0,
...

@jszeibert
Copy link
Contributor Author

sorry for the slow response, ... having some issues with my dev env, ...

@jszeibert
Copy link
Contributor Author

jszeibert commented Sep 5, 2023

I was finally able to do some digging about the attachment thumbnails or cover images sometimes not showing when uploading an image attachment to a card. This seems to be a nextcloud server core issue. There are similar issues reported. e.g. nextcloud/server#36463

The error message above is misleading as this isn't a permission issue but rather an incorrect error handling. The folder for the preview image can't be created, as it is already present. I added some debug output to the core to trace where the real error occurs (please disregard line numbers below as they might have shifted due to my debugging):

{ ...
   "method":"GET",
   "url":"/core/preview?fileId=558&x=260&y=100&a=1",
   "message":"mkdir(): File exists at lib/private/Files/Storage/Local.php#110",
...}
0 lib/private/Files/Storage/Wrapper/Wrapper.php(84): OC\Files\Storage\Local->mkdir('appdata_oc491gy...') 
#1 lib/private/Files/View.php(1148): OC\Files\Storage\Wrapper\Wrapper->mkdir('appdata_oc491gy...') 
#2 lib/private/Files/View.php(243): OC\Files\View->basicOperation('mkdir', '/appdata_oc491g...', Array) 
#3 lib/private/Files/Node/Folder.php(161): OC\Files\View->mkdir('/appdata_oc491g...') 
#4 lib/private/Files/AppData/AppData.php(149): OC\Files\Node\Folder->newFolder('1/b/b/9/1/f/7/5...') 
#5 lib/private/Preview/Storage/Root.php(74): OC\Files\AppData\AppData->newFolder('1/b/b/9/1/f/7/5...') 
#6 lib/private/Preview/Generator.php(643): OC\Preview\Storage\Root->newFolder('558') 
#7 lib/private/Preview/Generator.php(139): OC\Preview\Generator->getPreviewFolder(Object(OC\Files\Node\File)) 
#8 lib/private/Preview/Generator.php(116): OC\Preview\Generator->generatePreviews(Object(OC\Files\Node\File), Array, 'image/jpeg') 
#9 lib/private/PreviewManager.php(192): OC\Preview\Generator->getPreview(Object(OC\Files\Node\File), 260, 100, false, 'fill', NULL) 
#10 core/Controller/PreviewController.php(144): OC\PreviewManager->getPreview(Object(OC\Files\Node\File), 260, 100, false, 'fill') 
#11 core/Controller/PreviewController.php(113): OC\Core\Controller\PreviewController->fetchPreview(Object(OC\Files\Node\File), 260, 100, true, true, 'fill') 
#12 lib/private/AppFramework/Http/Dispatcher.php(230): OC\Core\Controller\PreviewController->getPreviewByFileId(558, 260, 100, true, true, 'fill') 
#13 lib/private/AppFramework/Http/Dispatcher.php(137): OC\AppFramework\Http\Dispatcher->executeController(Object(OC\Core\Controller\PreviewController), 'getPreviewByFil...') 
...

In this case OC\Files\Storage\Local->mkdir returns false which is passed down the stack to OC\Files\Node\Folder->newFolder throwing the misleading OCP\\Files\\NotPermittedException (see https://github.com/nextcloud/server/blob/master/lib/private/Files/Node/Folder.php#L162)


The issue is not persistent, as a refresh triggers the preview generation again and succeeds. Furthermore, I can't reproduce the issue on my new dev environment running nextcloud 28 server master build.


A fix that worked for me with my old dev environment (docker nextcloud images 27.0.2.1) was simply adding

	public function mkdir($path) {
		$sourcePath = $this->getSourcePath($path);
		$oldMask = umask($this->defUMask);
		$result = @mkdir($sourcePath, 0777, true);
		umask($oldMask);
+		if (is_dir($sourcePath)) { 
+			return true;
+		}
		return $result;
	}

at https://github.com/nextcloud/server/blob/master/lib/private/Files/Storage/Local.php#L114

@juliusknorr
Copy link
Member

I think there is some race condition as two different requests attempt to get the preview and probably both try to generate it for the first time.

Screenshot 2023-09-12 at 15 46 35

We could probably avoid this by having a blocking preview fetch request during the attachment upload before we add the attachment to the vuex store as a temporary fix. Other than that this is probably something to rather investigate in https://github.com/nextcloud/server/ to have some locking around that to avoid this kind of case in the preview generator.

@juliusknorr juliusknorr force-pushed the enh/CardCover branch 2 times, most recently from 5bb3965 to d3251f4 Compare October 17, 2023 10:34
jszeibert and others added 2 commits October 17, 2023 14:10
Signed-off-by: Johannes Szeibert <johannes@szeibert.de>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr merged commit 0b471b8 into nextcloud:main Oct 17, 2023
11 of 12 checks passed
@juliusknorr juliusknorr mentioned this pull request Oct 17, 2023
2 tasks
@jszeibert jszeibert deleted the enh/CardCover branch October 17, 2023 15:31
@juliusknorr juliusknorr self-assigned this Oct 25, 2023
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Images for Cards
3 participants