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

datetime: encapsulates datetime operations and add milliseconds #10844

Merged
merged 17 commits into from
Feb 11, 2022

Conversation

phweyland
Copy link
Contributor

Once done we can tweak datetime accuracy as needed.
First step to fix #10398

@phweyland phweyland added the scope: codebase making darktable source code easier to manage label Jan 9, 2022
@phweyland phweyland added this to the 4.0 milestone Jan 9, 2022
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

A first review. TIA.

src/common/datetime.c Outdated Show resolved Hide resolved
src/common/datetime.h Outdated Show resolved Hide resolved
src/iop/watermark.c Outdated Show resolved Hide resolved
src/common/variables.c Outdated Show resolved Hide resolved
src/control/jobs/control_jobs.c Outdated Show resolved Hide resolved
@phweyland
Copy link
Contributor Author

phweyland commented Jan 9, 2022

I could continue the next steps on the same PR, but as datetime usage is a bit spread over the code I would prefer to avoid too many rebases.

src/common/datetime.c Outdated Show resolved Hide resolved
Once done we can tweak datetime accuracy as needed.
First step to fix darktable-org#10398
- rework datetime.c
- no change on bd because sqlite disregards text size
@phweyland phweyland changed the title datetime: encapsulates datetime operations, in particular image datetime datetime: encapsulates datetime operations and add milliseconds Jan 21, 2022
@phweyland
Copy link
Contributor Author

phweyland commented Jan 22, 2022

Probably remain some bugs to be found out, but the main feature is there.

  • The $(EXIF_MSEC) variable is available at import and export time.
    • more changes on import side as time_t doesn't hold milliseconds
  • Milliseconds are read/written from/to database, Exif data, xmp data and xmp file.
    • to get them on images, 'resfresh exif'
  • Can be modified in geotagging module
    image
  • The bd doesn't change as sqlite ignores the text field sizing.
  • EDIT, if xmp, at import time, exif data are overwritten by xmp data and msec are not present.

I need some advice:

  • do we need to expose milliseconds in image information ? (Not done so far)
  • should they been hidden in collect module ? (BTW collections with one item do not make a lot of sense...)
    image
  • would we want to remove existing tm and time_t (unix time) occurrences ? (Remain a lot!)

I haven't changed watermark because it can access the standard variable (it should only use standard variables). If I touch to it, that will be to unify variables.

I still need to check the impact on collect, collection and timeline which access directly the bd datetime values.

Any comment or question is welcome.

+ if override date format invalid cancels importation
@johnny-bit
Copy link
Member

  • do we need to expose milliseconds in image information ? (Not done so far)

IMO yes

  • should they been hidden in collect module ?

IMO Yes. I agree that "single item collections" are not useful

  • would we want to remove existing tm and time_t (unix time) occurrences ? (Remain a lot!)

Ideally yes. Given that 2038 is "soon" and GNU Libc needs explicit param to enable 64bit time support, moving to glib time handling seems like a good idea overall. But maybe not in the scope of this PR. A lot of such occurences means a lot of rebases to do if pr is long-lived which may cause problems in the long run... So maybe separate PR after this gets merged and in several PRs to get changes in quickly and without need for lots of rebases :)

@TurboGit
Copy link
Member

  • do we need to expose milliseconds in image information ? (Not done so far)

Maybe only on a tooltip?

@phweyland
Copy link
Contributor Author

phweyland commented Jan 25, 2022

I've added a preference to show or not the milliseconds in "image information", but I'm wondering if it won't be better as global preference to control "geotagging" as well.
Not everybody needs them and that takes space.

EDIT: now controls image information and geotagging

@phweyland
Copy link
Contributor Author

Details...

  • Sqlite is able to manage iso-8601. So far dt follows exif standard, including in bd. This may simplify some queries involving datetime. I have not looked too much to timeline.c code but that may provide some advantage. @AlicVB, what do you think ?
  • g_date_time_format_iso8601() is available since glib 2.62 but dt requires 2.56 (exclusively).
  • The standard (iso-8601) separator between seconds and microseconds seems to be the dot (".") whatever the locale. However on the UI we could use the locale decimal character. Would anyone know how to get it based on the machine locale ?

@johnny-bit
Copy link
Member

  • g_date_time_format_iso8601() is available since glib 2.62 but dt requires 2.56 (exclusively).

the requirement for 2.56 comes from the fact that minimum supported ubuntu for 3.8 release was set to be 18.04 LTS. since that'll be over 4 y old by the time of 4.0 release I'd recommend switching minimum glib to 20.04 LTS which has glib 2.64

That way we'd also be able to drop need to support older compillers :)

@quovadit
Copy link
Contributor

it's awesome to have milliseconds as import-variable, thanks a lot!

Is there a chance to use $(EXIF_MSEC) only if $(EXIF_HOUR)$(EXIF_MINUTE)$(EXIF_SECOND) would not be unique?

@phweyland
Copy link
Contributor Author

phweyland commented Jan 28, 2022

Is there a chance to use $(EXIF_MSEC) only if $(EXIF_HOUR)$(EXIF_MINUTE)$(EXIF_SECOND) would not be unique?

Not sure to understand the question.
The main usage I can think about is to rename images taken in burst mode, then several ones by seconds.
But I don't see how to use it without the upper units, except if you know what you are doing (for example you know that all images are taken in the same second).
Other point, my Nikon has not a millisecond accuracy but gives only the 1/100 sec.

@quovadit
Copy link
Contributor

I would like to have unique filenames using date+time.
In most cases, it is enough to use yyyy-mm-dd_hh-mm-ss in order to be unique.
But sometimes there are multiple images in one second, so I need millisesonds.

In order to have short and readable filenames (in most cases) it would be great if millilseconds are only added to the filename if otherwise it would not be unique.

So using a variable $(EXIF_MSEC_IF_NOT_UNIQUE) would look like this:
2022-01-28_22-15-35.cr2
2022-01-28_22-15-37.cr2
2022-01-28_22-15-37-150.cr2
2022-01-28_22-15-37-892.cr2
2022-01-28_22-15-42.cr2

@phweyland
Copy link
Contributor Author

Got it !
Expand variables does its job properly but has no understanding of the variables meaning...
This said, it could have a retry mode in case of homonymy, and associated strategy for it, like either using an index or using optional variables as you suggest. But that's another story I'm afraid ...

@TurboGit TurboGit added documentation-pending a documentation work is required release notes: pending labels Feb 2, 2022
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

Maybe a better wording?

Apart from that it is all good to me.

data/darktableconfig.xml.in Outdated Show resolved Hide resolved
data/darktableconfig.xml.in Outdated Show resolved Hide resolved
@TurboGit
Copy link
Member

Sorry Philippe but you have also some conflicts here!

@TurboGit TurboGit merged commit d9e582d into darktable-org:master Feb 11, 2022
@TurboGit
Copy link
Member

@phweyland : The conflict was easy to resolved, I've done so and merged this. Thanks again!

@phweyland phweyland deleted the datetime-encap branch February 15, 2022 17:43
@phweyland phweyland added the documentation-complete needed documentation is merged in dtdocs label May 19, 2022
@elstoc elstoc removed the documentation-pending a documentation work is required label May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-complete needed documentation is merged in dtdocs scope: codebase making darktable source code easier to manage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Do we need to get rid of the unix time limitations ?
6 participants