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

Feature/caption and title (#14) #870

Merged
merged 6 commits into from
Apr 4, 2024
Merged

Feature/caption and title (#14) #870

merged 6 commits into from
Apr 4, 2024

Conversation

grasdk
Copy link
Contributor

@grasdk grasdk commented Mar 31, 2024

  • reading caption and title from several tags with several tests
  • added minor bugfix for date conversion to MS
  • optimization: break sidecar loop, when first sidecar is found

grasdk and others added 2 commits March 31, 2024 14:57
* reading caption and title from several tags with several tests added
minor bugfix for date conversion to MS
optimization: break sidecar loop, when first sidecar is found
@grasdk
Copy link
Contributor Author

grasdk commented Mar 31, 2024

Ok. Once again I'm struck by dates. The error has nothing to do with this PR.

The problem lies in these lines:
https://github.com/bpatrik/pigallery2/blob/master/src/backend/model/database/SearchManager.ts#L769-L790

When running the tests today, the 31st of march, the subtraction of months behaves weirdly. Javascript data handling is notoriously bad.

Take a look here, how subtracting a month from the 31st of march (or 1st of april) suddenly becomes the 3rd of march:

>to = new Date();
Date Sun Mar 31 2024 17:39:12 GMT+0200 (Central European Summer Time)

>to.setHours(0, 0, 0, 0);
1711839600000
>to.setUTCDate(to.getUTCDate() + 1);
1711926000000
>to
Date Mon Apr 01 2024 01:00:00 GMT+0200 (Central European Summer Time)

>to.setUTCMonth(to.getUTCMonth() - 1);
1709420400000
>to
Date Sun Mar 03 2024 00:00:00 GMT+0100 (Central European Standard Time)

>to.setUTCMonth(to.getUTCMonth() + 1);
1712098800000
>to
Date Wed Apr 03 2024 01:00:00 GMT+0200 (Central European Summer Time)

So in javascript if you subtract a month and then add a month, you won't always arrive at the starting point. Especially around end of longer months.

I will try and fix the tests. They time out when they run locally, so I have to commit and sync every time I think I have a solution.

@bpatrik
Copy link
Owner

bpatrik commented Apr 1, 2024

Ahh this search is annoying.

I would just create a PR for the original feature. The code looks good to me so far. If metadata test runs, I will merge it.

Then we can have a separate discussion or at least a bug about the date search.

@grasdk
Copy link
Contributor Author

grasdk commented Apr 1, 2024

Ok. I think my solution to the search is more intuitive than the default js one.

Basically it is:
If (date+x months)'s day of month does not exist, change to the largest day of month.

  • So 31st of april, june, september and november are converted to 30th.
  • 30th and 31st of february are converted to 29th on leap years
  • 29th, 30th and 31st of February are converted to 28th of non-leap years

Note, month can be negative, so you can add -1 month, as the search does:

So 31st of march one month ago and 3 days back means, 27th, 28th and 29th of February this year.
Before my change (default js behavior) it was 1st, 2nd and 3rd of march this year.

Of course, I can remove this easily from the PR if you would like that. Can do it tonight. :-)

@grasdk
Copy link
Contributor Author

grasdk commented Apr 1, 2024

I reverted not only the tests but also the other search-mechanism described above.

I will resubmit in another PR, so you can try it out separately :)

Edit: today the tests run fine because it's the 1st of April ;)

@bpatrik bpatrik added this to the Next (probably v2.5) milestone Apr 4, 2024
@bpatrik
Copy link
Owner

bpatrik commented Apr 4, 2024

Thank you!

@bpatrik bpatrik merged commit 6e85dc4 into bpatrik:master Apr 4, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants