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.

Sign up for GitHub

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

Jump to bottom

Add thumbnails for files in the activity view #4189

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Add thumbnails for files in the activity view #4189

merged 1 commit into from
Mar 17, 2022

Conversation

claucambra
Copy link
Collaborator

This PR adds thumbnails for file changes in the activity list.

image

Partially addresses #3666

@claucambra claucambra self-assigned this Jan 21, 2022
@claucambra claucambra force-pushed the feature/activity_thumbnails branch from ed346af to 855e6bb Compare January 21, 2022 11:36
CarlSchwan
CarlSchwan reviewed Jan 22, 2022
View reviewed changes
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Cool feature. Could you also test it with previews disabled from the server side?

src/gui/tray/ActivityItem.qml Outdated
width: parent.width * 0.66
height: parent.height * 0.66
anchors.centerIn: parent
cache: true
Copy link
Member

Choose a reason for hiding this comment

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

Can you make sure that the cache works across restart? Generating a thumbnail is a bit expensive on the server side.

Copy link
Collaborator Author

@claucambra claucambra Jan 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Eyeballing it, it seems to work across computer restarts but I'm not sure, is there some way to tell if QML is loading the image from cache/if the server is receiving a GET request for the thumbnail?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@CarlSchwan are those previews cached on server side ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

so the cache is not kept after a restart
I always see that we download the preview images

@claucambra
Copy link
Collaborator Author

Cool feature. Could you also test it with previews disabled from the server side?

Yeah, instead of previews the preview API presents thumbnails corresponding to the file type:

Screenshot from 2022-01-24 15-21-41

@claucambra claucambra force-pushed the feature/activity_thumbnails branch from e13ce96 to 10a2b60 Compare January 25, 2022 10:54
@claucambra claucambra changed the title Draft: Add thumbnails for files in the activity view Add thumbnails for files in the activity view Jan 25, 2022
@claucambra claucambra force-pushed the feature/activity_thumbnails branch 3 times, most recently from d8748a1 to 0b9a680 Compare January 27, 2022 17:42
@jancborchardt
Copy link
Member

Yeah, really great! Only a small enhancement: Could you give the image thumbnails 3px border radius so it's the same as on server?

@mgallien
Copy link
Collaborator

mgallien commented Feb 3, 2022

@claucambra this PR needs a rebase to solve the existing conflicts

@claucambra
Copy link
Collaborator Author

Yeah, really great! Only a small enhancement: Could you give the image thumbnails 3px border radius so it's the same as on server?

Sure, here's a screenshot of the change:

image

mgallien
mgallien reviewed Feb 3, 2022
View reviewed changes
src/gui/systray.cpp Outdated Show resolved Hide resolved
src/gui/tray/ActivityItem.qml Outdated
width: parent.width * 0.66
height: parent.height * 0.66
anchors.centerIn: parent
cache: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

so the cache is not kept after a restart
I always see that we download the preview images

mgallien
mgallien requested changes Feb 7, 2022
View reviewed changes
src/gui/tray/activitydata.cpp Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/usermodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/usermodel.cpp Outdated Show resolved Hide resolved
test/testactivitydata.cpp Outdated Show resolved Hide resolved
camilasan
camilasan reviewed Feb 7, 2022
View reviewed changes
Copy link
Member

@camilasan camilasan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Merge branch 'master' into feature/activity_thumbnails

Please rebase, not merge.

jancborchardt
jancborchardt requested changes Feb 8, 2022
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Good stuff @claucambra, just 2 more things:

  • The thumbnails seem a bit small, could you increase the size of them so that their left edge lines up with the left edge of the search field (of course they should still be centered, so they will also extend to the right as well as top and bottom)
    • Maybe to make it look nice, the mimetype icons also have to be made larger a bit if possible – or is that their native pixel size from the SVG so they don’t get blurry?
    • (In the screenshot below, the "Tasks" icon for example looks good, and the thumbnails too small)
  • All text should left-align with the username in the header, so the "All synced" status and Search field text needs to move a bit right, and the text of the entries a bit to the left

desktop tray thumbnails

@claucambra
Copy link
Collaborator Author

claucambra commented Feb 11, 2022 •
edited
Loading

Nice. I'd approve after Jan gives a go design-wise

@jancborchardt is there anything else I should address in this PR? :)

imagen

@claucambra claucambra requested a review from camilasan February 16, 2022 11:04
@mgallien
Copy link
Collaborator

mgallien commented Mar 9, 2022

@claucambra can you rebase it on top of master ?
there are conflicts

mgallien
mgallien reviewed Mar 9, 2022
View reviewed changes
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

a few comments and we should be good to go
sorry for the delay

src/gui/tray/ActivityItemContent.qml Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.h Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.cpp Show resolved Hide resolved
@claucambra claucambra force-pushed the feature/activity_thumbnails branch from a8b4aa6 to 74e2379 Compare March 9, 2022 10:44
@claucambra claucambra requested a review from mgallien March 9, 2022 10:57
@claucambra claucambra force-pushed the feature/activity_thumbnails branch from 988ae6d to 25545cf Compare March 9, 2022 12:15
@claucambra claucambra requested a review from mgallien March 9, 2022 12:19
@PhilippSchlesinger PhilippSchlesinger mentioned this pull request Mar 10, 2022
Closed
3 tasks
@claucambra claucambra force-pushed the feature/activity_thumbnails branch from 25545cf to a885d71 Compare March 15, 2022 12:14
mgallien
mgallien reviewed Mar 15, 2022
View reviewed changes
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

small extra comments

src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/activitydata.cpp Outdated Show resolved Hide resolved
@claucambra claucambra force-pushed the feature/activity_thumbnails branch from 8d2543f to 3cd784c Compare March 16, 2022 09:48
@mgallien
Copy link
Collaborator

@claucambra can you clean history before approval ?

@claucambra claucambra force-pushed the feature/activity_thumbnails branch from 3cd784c to 592e1aa Compare March 16, 2022 12:48
@claucambra
Copy link
Collaborator Author

@claucambra can you clean history before approval ?

Yup, squashed

@claucambra claucambra requested a review from mgallien March 16, 2022 12:49
@claucambra
Copy link
Collaborator Author

@jancborchardt Are there any other changes that need to be made before approval? :)

@claucambra
Copy link
Collaborator Author

/rebase

@claucambra
Add thumbnails for files in the activity view
65f2bad 
Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com>
@nextcloud-command nextcloud-command force-pushed the feature/activity_thumbnails branch from 592e1aa to 65f2bad Compare March 17, 2022 10:46
@nextcloud-desktop-bot
Copy link

AppImage file: Nextcloud-PR-4189-65f2bada3e3095eb88c2e9b494ff8b87a72b5227-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

jancborchardt
jancborchardt approved these changes Mar 17, 2022
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks all good! :)

@claucambra claucambra merged commit 6aefe8f into master Mar 17, 2022
@claucambra claucambra deleted the feature/activity_thumbnails branch March 17, 2022 10:57
@sonarcloud
Copy link

sonarcloud bot commented Mar 17, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

37.4% 37.4% Coverage
0.0% 0.0% Duplication

@mgallien mgallien added this to the 3.5.0 milestone Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlSchwan CarlSchwan CarlSchwan left review comments

@allexzander allexzander allexzander approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@mgallien mgallien mgallien approved these changes

@camilasan camilasan Awaiting requested review from camilasan

Assignees

@claucambra claucambra

Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.
7 participants
@claucambra @jancborchardt @mgallien @allexzander @nextcloud-desktop-bot @camilasan @CarlSchwan