-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fully configurable metadata module #18036
Conversation
I'll test the lua part. |
Just picking out one random thing (not pretending at all that this amounts to anything like the in depth review this big PR deserves), but why don't you create one global |
Started testing. Created a fresh environment and pulled the PR. Imported a directory of play raws. Tested a lua script to add a field to the image information module. It added the field, but it didn't show up in the preferences and I couldn't control the visibility, which I can with current master. Stopped darktable, then restarted it. The collection of play raws would not display. Went to the collections module and double clicked on the play raw collection and it still wouldn't display. I'll continue testing... |
Along with not displaying the field in the preferences, it seems to cause a crash when exiting, which is what stopped the collection from displaying since darktable did not shut down cleanly. The error:
Here's the script: drop it in your contrib lua scripts directory and remove the .txt extension. start darktable, then start the script. It adds a module to lighttable below image information. In the metadata display name field enter In the tag field enter Click create and The script requires the exiv2 program be installed and in your path. Hover over images and it should update. Just tested again, and I deactivated the field (removed it from the image information display) and didn't get a crash. On another note. How can I notify Lua of the changed collection fields, since I can control collections from Lua. |
@wpferguson thanks for testing so far, I will look into that (I know near to nothing about Lua). The crash on exit is another story: #18023 |
@dterrahe: I had to read that twice but now I got your point. The variables completion was indeed somewhat complicated and it was the last part I have implemented in this PR. Your suggestion sounds good, I will work on that. |
1b303b6
to
f48d993
Compare
@dterrahe: I followed your suggestion, works great! So I could completely remove the signal handlers for the variables completion. |
@wpferguson: I think I have fixed the Lua issues in the image information module, can you please test again?
In the collection module I have written the code to register the collections. Can you give me some advice/script on how to test this? |
bf197af
to
bb64eb1
Compare
Using a script if I have a metadata active in the image information display, then stop and start darktable, the metadata doesn't appear in the image information display, nor in the preferences. I have to "remove it" then reactivate it to get it to appear. EDIT: also getting
in the terminal EDIT 2: After further testing, I can't reproduce the doesn't display after reload. |
@wpferguson: Thanks for testing! I have fixed the glib critical messages.
I have noticed that too, but independent of this PR. I get the same behavior on master. Seems to be an issue with your lua script? |
@zisoft : No urgency, but this one also needs a conflict resolution. TIA. |
rebased and force-pushed. |
7ab8269
to
6a8a6b3
Compare
First test on my side, sounds promising... One issue I found on the dialog to name the new field: Where you have the X, I want to rename the field. I click on it, can edit but the new value is not saved if I:
The only way I found if after changing the text, clicking on another entry and then click |
Yes, I have noticed that too. One need to first remove the focus from the edit edit field by clicking on another entry. I have tried to capture the focus change event but no success. The cancel edit event is always executed first. Maybe @dterrahe has any idea how to solve this? |
@zisoft : IIRC there is something similar done in the metadata lib module. See when you enter a value in the entry it gets recorded when you type |
I suppose you want to save the data when the entry leave focus, and do the same when typing Enter (you probably want a new callback on "key-press-event". See |
GitHub is showing +23,748 −22,787 changes. Are all line-ending for all touched files changed? This may stand in the way of usefulness of git blame. Similarly will the 41 commits be (partly) squashed before merging, to benefit git bisect? In cases of (partial) reverts especially. |
Yes, seems like some files have the whole content changed... Is that an end-of-line from UNIX style to something else? |
Indeed, the line endings are messed up. Revert 57c482e, fix the line endings and commit again? |
This should work I suppose. |
Ok, I think I have fixed this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working for me, tested with a current db and a clean one. Thanks a lot!
Release notes: The metadata module is now fully configurable and allows to add and maintain any tags which are supported by exiv2. |
@TurboGit so I guess you don't think merging 41 separate commits, some of them known broken, is going to discourage bisecting for a while? |
@dterrahe : No, you guessed wrong :), this is annoying but after looking at the commits it seemed to me that it would be hard to clean-up all this. For bisect one can skip steps if needed. |
Understood, but I'd prefer in those cases to have all of them squashed into 1 commit, after duplicating all the individual commits into a separate branch. If there turns out to be an issue that traces back to this PR, the main developer (and anybody looking at their fork) can still access the further breakdown and do an additional bisect within that, but with the knowledge that some of the intermediate steps in that bisects will be broken for known reasons. It is not useful for a user to find all kinds of distracting additional issues when their bisect step presents them with a partially working state. And the number of bisect steps will also be uselessly (for them) large. It might deteriorate the quality of bug reports related to this, and potentially other unrelated issues, and might permanently discourage them from attempting bisects in the future, which would be a real loss! Anyway, too late now, but something to keep in mind in future. I very much consider git history as part of (or our only) documentation so making it less useful makes development harder. It takes much more effort to determine the original reason for code and thereby to avoid regressions. It also makes it less fun (if there was any left) to dig through it. |
This PR implements a fully configurable metadata module. It allows to maintain every tag which is supported by exiv2.
fixes #13607
fixes #17664
fixes #18147
The metadata tags are maintained in a new database table
data.meta_data
, the previous darktable tags are created at database migration.So the metadata module looks exactly like before:
The metadata preferences dialog now shows the tagname and has buttons to add new tags and to delete tags:
Clicking the
+
button to add new tags opens the tag dialog which is already known from the export module:Search for the desired tags and add them by either double click or the
add
button. In this example the user wants to add theXmp.iptc.AltTextAccessibility
andXmp.iptc.Location
tags. Clickdone
and the tags are added to the metadata preferences dialog.Edit the display name:
and drag the tag to change the order to your liking:
Click
save
and the metadata module is immediately updated:As well as the metadata section of the import module:
The new tags can immediately be used in the collections module:
...and the image information module (this module has its own preferences for show/hide and display order):
Finally, the new tags can now be used as variables: