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

Use ExifToolWrapper with stayopen for parsing DirContents #52

Merged
merged 10 commits into from
Jan 21, 2023

Conversation

Urmel
Copy link
Contributor

@Urmel Urmel commented Jan 9, 2023

Current state of work - speed wise it looks promising.

UTF8 thing should be covered via way the ExifWrapper is doing things, but must still be tested + further changes to support all columns ...

@Urmel
Copy link
Contributor Author

Urmel commented Jan 11, 2023

  • All columns should work now
  • First test with "Umlaute" successful

Issue with Lens and ISO columns - likely SideCar file is not yet respected
...

@Urmel
Copy link
Contributor Author

Urmel commented Jan 11, 2023

Regarding UTF8 - I had to remove forcing exiftool to interpret internally the image's metadata as UTF8. Only the filename and StdOut is set to use UTF8. Then it worked...

@Urmel
Copy link
Contributor Author

Urmel commented Jan 13, 2023

Build from #48 compared with latest commit shows identical data.

For the three nef with lens data, XMP sidecars are in the folder.

Base version:
grafik

Changed:
grafik

Page 2 Base Version:
grafik

Page 2 Changed:
grafik

I've also briefly tested using the editform - works as well.

Open:

  • Make the text black
  • get rid of the double extension on the nef files
  • Removal of not anymore needed code (previous EXIFHelper methods only relevant for reading, etc)
  • further testing

@Urmel
Copy link
Contributor Author

Urmel commented Jan 14, 2023

  • Made text in file list view black again
  • Fixed double extension bug
  • Removed obsolete EXIFHelper method
  • Tested some more and fixed some things
  • Rebased on top current development branch

Chinese chars seem to work as well:
grafik

@Urmel Urmel marked this pull request as ready for review January 14, 2023 22:16
@Urmel
Copy link
Contributor Author

Urmel commented Jan 14, 2023

Update - as the order in which the tags were used was wrong for country (XMP was behind IPTC).

@nemethviktor
Copy link
Owner

Hi @Urmel just a quick comment.
I've pushed some commits just now to Dev. I'm very consciously trying not to interfere with the changes here (so far so good I think) -- just a heads up. The objectNames sqlite/xlsm has been changed a little because I think there was a change in exifTool itself (unsure tbh) and I discovered that 3 new rows are required.

  • IPTC:Province-State
  • IPTC:Country-PrimaryLocationName
  • IPTC:Country-PrimaryLocationCode

Can you double check this is still ok in the "model"-based world?

@Urmel
Copy link
Contributor Author

Urmel commented Jan 17, 2023

Hi @nemethviktor,
thanks for being considerate of this, rebasing can be a pain ;)
I don't get what actually the change is - i.e where the 3 new rows are required. Looking at the XLS in previous and latest version, the objectMapping_IN did not change?
grafik

The three mentioned tags are used in SourcesAndAttributes.TagsToAttributesOrder:

                { ElementAttribute.State, new List<string>() {
                    "XMP:State",
                    "IPTC:Province-State"
                } },
                { ElementAttribute.Country, new List<string>()
                {
                    "XMP:Country",
                    "IPTC:Country-PrimaryLocationName"
                } },
                { ElementAttribute.CountryCode, new List<string>() {
                    "XMP:CountryCode",
                    "IPTC:Country-PrimaryLocationCode"
                } },

So the mapping for "in" (i.e. reading EXIF Tags) did not change?

Thanks!

@nemethviktor
Copy link
Owner

It's the out part.
TLDR is that when using (in dev version as of say a day or two ago [ie not newest newest] the "clear data" button, some strings remained, which i think were "new" as i had tested that functionality in the past and at the time it had worked.

Does that help?

if (_ExifTool == null)
throw new InvalidOperationException($"Cannot scan a folder (currently '{folder}') when the EXIF Tool was not set for the DirectoryElementCollection.");

if (folder.EndsWith(value: @"\"))
Copy link
Owner

@nemethviktor nemethviktor Jan 18, 2023

Choose a reason for hiding this comment

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

Any particular reason for introducing this one?
This actually seems to cause a problem. I have a bit of an obscure setup on my computer but the logic is that the app sits on E:\Google Drive\Sajat\Jobs\Nem_Aktualis\Apps\GeoTagNinja\ - this folder (technically all of E:\Google Drive\*.*) doesn't entirely exist though, it's a junction based mirror of the G:\ (which is Google Drive, because when Google initially intro'd the then-new version of GD there wasn't an option to keep GD files anywhere other than their own drive letter, so I hacked it) - when we further down call string tmpStrParent = HelperStatic.FsoGetParent(path: folder); it will try to get the parent of E: and return a very odd path instead, in particular the debug (!!!) subfolder of the app - E:\Google Drive\Sajat\Jobs\Nem_Aktualis\Apps\GeoTagNinja\bin\Debug\. So I can't navigate to E:
When we keep the \ [as in E:\] the app works okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually just copy-pasted from the previous method HelperStatic.ExifGetExifFromFolder(string folderNameToUse). There, it read:

    if (folderNameToUse.EndsWith(value: @"\"))
    {
        folderNameToUse = folderNameToUse.Substring(startIndex: 0, length: folderNameToUse.Length - 1);
    }

I guessed that it should ensure that there is no path used like "E:\folderA"... :)

That d... thing behaves differently - not nice:
grafik

But looking at the result C:\Temp\ versus C:\Temp, we should be fine by just removing this removal:
grafik

@Urmel
Copy link
Contributor Author

Urmel commented Jan 19, 2023

It's the out part. TLDR is that when using (in dev version as of say a day or two ago [ie not newest newest] the "clear data" button, some strings remained, which i think were "new" as i had tested that functionality in the past and at the time it had worked.

Does that help?

When it just relates to the out part. This should not affect the code in this PR....
I don't get a grasp on the issue yet - with "clear data", you mean the "Edit Form Clear Location Data Button"? Or the right-click "Remove Cached Data"? Where did the strings remain?
Thanks

@nemethviktor
Copy link
Owner

"Clear Data" -> screenshot

image

@Urmel
Copy link
Contributor Author

Urmel commented Jan 19, 2023

Regarding your comment on performance:

Particular to Large Number of Folders Slower --> parsing the list of 266 subfolders (no files at all) takes about 6.9 seconds. In the current dev version it's 1.1 sec. :)

This is reproducible. The performance is not lost in reading or EXIFing, but in setting up the list view. There is a screen update every item...

I solved it by removing the updates while adding items to the list view. The longest time is spent parsing EXIF data, which has no updates to the list view. The actual adding of items then is quite fast if we do not explicitely jump to every item added ;)

@Urmel
Copy link
Contributor Author

Urmel commented Jan 19, 2023

"Clear Data" -> screenshot

image

Ok. Thanks. I understand what you mean I guess:
grafik

At least the data in the ref columns (long, lat, alt) is left. But these get erased:
grafik

At first sight, I do not get the connection between the PR and this behaviour, as the PR "stops" after data was filled into the list view and the backend tables. But I can look into it...

@Urmel
Copy link
Contributor Author

Urmel commented Jan 19, 2023

Fixed the Folder Up issue and the long load times of a folders only folder with latest commit...

@nemethviktor
Copy link
Owner

Sorry I wasn't clear enough. The "remove data" issue wasn't due to the code in the PR....hence my recent update to the objectmapping sqlite file.

@nemethviktor
Copy link
Owner

Vaguely related but I'm thinking since the other issues (tickets, the ones unrelated to stay-open and DE) are solved I could push current Dev branch to master and afterwards merge the pr to Dev? It's probably make your life easier (or harder? Any preference?)

@Urmel
Copy link
Contributor Author

Urmel commented Jan 20, 2023

Hi @nemethviktor,
thanks for the update.
I guess so - if this PR is the way we want to go, we should merge. There has been testing from you and me. You seem to even have a actual test list that you can work on.
Merging will make my life easier in terms of - no more rebasing of this PR :) It will likely also change your life, as you might be involved in further bug fixing or features using the concepts introduced here.... ;)

@nemethviktor
Copy link
Owner

I'll move things around tomorrow most likely (aka merge)
Haha I don't have a test-bed, I have about 100k photos lol and I'm using the app on it/them. :) -- but I do test a bit on various VMs too.

Key changes to other parts of project
* Added convenience function AllCompatibleExtensionsExt
* Adapted lvwFileList_LoadOrUpdate to support update concept via DirectoryElement and ListView
* Moved ParseCurrentDirectoryToDEs to DirectoryElementCollection

Signed-off-by: Alexander Behring <alex@behrings.eu>
* First test with "Umlaute" successful
* Issue with Lens and ISO - likely SideCar file is not yet respected
* Added property SidecarFile to DirectoryElement
* Updated ParseFolderToDEs to handle sidecar files
* Removed doubled file endings
* Removed GetExifFromFolder method (obsolete)
* Bugfixing (missed DtFileDataSeenInThisSession.Clear,
  Conversion of Taken, CreatedDate and ExposureTime
  and other minor ones)
* Enabled CheckForVersion again (was commented out for testing)
* made AttributeValueContainer internal classes
* Code Commenting

Signed-off-by: Alexander Behring <alex@behrings.eu>
Signed-off-by: Alexander Behring <alex@behrings.eu>
* EXIFTool now gets started with AppStart and disposed during AppClosing
* Reduce Label Updates during parsing (only every 10th for parse, every 20th for display)
* Fixed missing "." dot in Filename when WindowsExplorer does not show FileExtensions
Signed-off-by: Alexander Behring <alex@behrings.eu>
* Fixed "up to parent" resulted in "working path of
  application" when afterwards clicking on c:
@Urmel
Copy link
Contributor Author

Urmel commented Jan 20, 2023

Rebased it onto top of current dev branch.

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