Card Cover Images #5035
Card Cover Images #5035
Conversation
|
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? :) |
|
Translations do not need to be done in the pull request as long as all code parts use the proper |
There was a problem hiding this comment.
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
Outdated
| cardId: { | ||
| immediate: true, | ||
| handler() { | ||
| this.fetchAttachments(this.cardId) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I still haven't come up with a good idea for that.
One option might be to approach this similar to the attachment count:
deck/lib/Cache/AttachmentCacheHelper.php
Line 49 in 03f4006
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
|
jszeibert
force-pushed
the
enh/CardCover
branch
from
1d5454f
to
cefb637
Compare
Yes, that is a strange one, ... either the thumbnail in the Attachment list or the Card Cover isn't rendered on upload. The error on nextcloud server 27.0.2.1 is: |
|
sorry for the slow response, ... having some issues with my dev env, ... |
|
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): In this case 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 |
|
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.
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
force-pushed
the
enh/CardCover
branch
2 times, most recently
from
5bb3965
to
d3251f4
Compare
Signed-off-by: Johannes Szeibert <johannes@szeibert.de>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
juliusknorr
force-pushed
the
enh/CardCover
branch
from
d3251f4
to
24b7c23
Compare
|
Hello there, 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! |
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):
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.
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.
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:
TODO
Checklist