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

Implement 'On this day' #937

Merged
merged 4 commits into from
Jan 14, 2022
Merged

Implement 'On this day' #937

merged 4 commits into from
Jan 14, 2022

Conversation

marcelklehr
Copy link
Member

fixes #179

image

  • Uses the standard Timeline component
  • Goes back 20 years
  • Displays photos from this day and "tomorrow" for convenience (so overnight events still work as expected)

@jakobroehrl
Copy link
Contributor

Yeah, freaking cool! Waited so long for this!
What do you think of

  1. extend the range to 1-2 days in the past and 1-2 days in the feature from now?
  2. if there is no catch extend the range even more. Show some photos from the month of the past years e.g.? Discover: Show memories like today x years ago photoprism/photoprism#720 (comment)
  3. call it moments or recommendations?

@jakobroehrl

This comment has been minimized.

Sign in to view
@marcelklehr
Copy link
Member Author

marcelklehr commented Dec 16, 2021 •
edited
Loading

Yeah, freaking cool! Waited so long for this!

Merry Christmas 🎄 🎅

  1. extend the range to 1-2 days in the past and 1-2 days in the feature from now?
  2. if there is no catch extend the range even more. Show some photos from the month of the past years e.g.? Discover: Show memories like today x years ago photoprism/photoprism#720 (comment)

We can definitely discuss the date range. I'm not particular about this.

  1. call it moments or recommendations?

Not particular about the name either. :)

What do you think about a notification (off per default) if there was a photo found of this day in the past?

I'm more in favor of a dashboard widget, personally :)

// cc @nextcloud/designers

@jakobroehrl
Copy link
Contributor

What do you think about a notification (off per default) if there was a photo found of this day in the past?

I'm more in favor of a dashboard widget, personally :)

Good idea, it's better with a dashboard widget

@jakobroehrl

This comment has been minimized.

Sign in to view
@marcelklehr

This comment has been minimized.

Sign in to view
Mikescops
Mikescops reviewed Dec 17, 2021
View reviewed changes
src/services/PhotoSearch.js
? `<d:or>${Array(20).fill(1).map((_, years) => `<d:and>
<d:gt>
<d:prop>
<d:getlastmodified />
Copy link
Member

Choose a reason for hiding this comment

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

the behavior will be weird, cause lastmodified is impacted when you tag an image for instance, so it won't be exactly "on this day"

but I guess that's the best we can do with Dav

Copy link
Member Author

Choose a reason for hiding this comment

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

Once nextcloud/server#30366 is merged this could be changed to creation_time

Copy link
Member

Choose a reason for hiding this comment

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

ah interesting, I always thought this would cause performance issues

@skjnldsv am I wrong?

@szaimen szaimen added this to the Nextcloud 24 milestone Dec 20, 2021
@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature or request labels Dec 21, 2021
@jakobroehrl
Copy link
Contributor

@marcelklehr
Do you create the dashbaord widget for pictures in this PR, too?

@marcelklehr
Copy link
Member Author

Do you create the dashbaord widget for pictures in this PR, too?

Nope.

@skjnldsv skjnldsv requested review from jancborchardt and nimishavijay and removed request for skjnldsv December 27, 2021 06:48
@jancborchardt
Copy link
Member

Awesome @marcelklehr! So design-wise, I was thinking it could be in the first row of "Your photos", similarly to the recommendations in Files, or how we sort favorites up top. But after a very quick mock-up, I’m actually not sure anymore. :D
image

I’d say it’s good how you did it – just thoughts about naming and timespan:

  • The headings might be better as: "1 year ago December 2020", otherwise it’s always December December December etc. ;)
  • The naming is "On this day", but we should probably also fuzzily include photos a few days around the date, right? Cause it’s probably rare for people to take a photo every single day, and then often "1 year ago" would just be nothing. Maybe then even we rename it to "Through the years" or "Memories" or something similar?
  • I assume that it’s starting in 2021 is just to show as an example? Cause it would start at 1 year ago, right?

@marcelklehr
Copy link
Member Author

marcelklehr commented Dec 29, 2021 •
edited
Loading

Thanks for the review!

The headings might be better as: "1 year ago December 2020", otherwise it’s always December December December etc. ;)

Fixed

The naming is "On this day", but we should probably also fuzzily include photos a few days around the date, right? Cause it’s probably rare for people to take a photo every single day, and then often "1 year ago" would just be nothing. Maybe then even we rename it to "Through the years" or "Memories" or something similar?

I've changed it now to also include 3 days before and 3 days after "today".

"Memories" is a bit too generic, IMO, "Through the years" is nice but lacks the "current date" aspect. So far, I like "On this day" best, even though it may not always display photos from exactly the same date.

I assume that it’s starting in 2021 is just to show as an example? Cause it would start at 1 year ago, right?

My bad. Fixed now :)

New screenshot:

image

@Spartachetto
Copy link

First of all thank you very much, cool feature.

A long term suggestion: once nextcloud/server#30416 will be included, some other possibilities could be considered. For example using the geo data and showing the photo farthest from your usual location

@jancborchardt
Copy link
Member

Nice @marcelklehr! :) Only one detail, the heading bolding could be improved to what I mentioned:

1 year ago · January 2021

Specifically the "x years ago" part bold, nd the "Month 20xx" part not bold.

Additionally to what I mentioned before, for spacing there could be a middle dot inbetween with 2 spaces to either side.

@marcelklehr
Copy link
Member Author

Specifically the "x years ago" part bold, nd the "Month 20xx" part not bold.

Ah. I missed that.

Here we go:
image

jancborchardt
jancborchardt approved these changes Jan 5, 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 awesome! :) Great work @marcelklehr

@marcelklehr
Copy link
Member Author

/compile amend /js

@marcelklehr
Copy link
Member Author

This branch has conflicts that must be resolved

🤔

@skjnldsv Did I do something wrong?

@skjnldsv
Copy link
Member

This branch has conflicts that must be resolved

thinking

@skjnldsv Did I do something wrong?

No, I had to merge another big PR bumping dependencies, you need to rebase and drop js changes.

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 14, 2022
@skjnldsv skjnldsv force-pushed the feature/on-this-day branch from 2e9cd2f to 2a2a06d Compare January 14, 2022 08:01
@skjnldsv
Copy link
Member

Rebased

@skjnldsv skjnldsv merged commit aec6dfc into master Jan 14, 2022
@skjnldsv skjnldsv deleted the feature/on-this-day branch January 14, 2022 08:20
@galandilias
Copy link

Great feature :) what do you think about implementation of a plugin to dashboard showing a preview from 'on this day'? I would love it as this is my main landing page when working with Nextcloud :)

@skjnldsv
Copy link
Member

Great feature :) what do you think about implementation of a plugin to dashboard showing a preview from 'on this day'? I would love it as this is my main landing page when working with Nextcloud :)

Could you open an issue about this? :)
Great idea!

@galandilias
Copy link

there you go:
#972

@count00zero
Copy link

It seems to be pushed to "master", but is there an additional step to enable it in the standard photos-app within NC 23.0.2?
Cause I can't "see" the feature ...

@jakobroehrl
Copy link
Contributor

It will be released with the next feature release, this is 24.
23.0.2 is only for bugfixing

@count00zero
Copy link

got it, thanks!

@marcelklehr marcelklehr mentioned this pull request Jun 29, 2022
Merged
@jakobroehrl jakobroehrl mentioned this pull request Sep 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikescops Mikescops Mikescops approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@jakobroehrl jakobroehrl Awaiting requested review from jakobroehrl

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request
Projects
None yet
Milestone
Nextcloud 24
Development

Successfully merging this pull request may close these issues.

Memories / Show pictures taken previous years on this day
9 participants
@marcelklehr @jakobroehrl @jancborchardt @Spartachetto @skjnldsv @galandilias @count00zero @Mikescops @szaimen