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

Rename to allow all exiv2 tags as placeholders #2791

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

norbertwg
Copy link
Contributor

@norbertwg norbertwg commented Oct 9, 2023

closes #2238

  • The syntax: the tag name has to be enclosed in colons.
  • The placeholder will be replaced by translated value, not by the plain value.
  • Characters in tag's translated value, which are not allowed in file names, will be replaced by underscore.

@ghost
Copy link

ghost commented Oct 9, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@norbertwg
Copy link
Contributor Author

When replacing characters, which are invalid in file names, it is currently only distinguished between Windows and others. I tried to separate Mac OS and Linux/Unix as well, but I am not sure how to do it. In XMP_environment.h I found a line of code, but it was commented with /* Todo: How to correctly recognize a Mac platform? */. The same problem I have with the test case. If somebody can give me a hint, I can refine the logic to differentiate the operating systems.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #2791 (a2b60a4) into main (fe44c8c) will increase coverage by 0.10%.
Report is 5 commits behind head on main.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##             main    #2791      +/-   ##
==========================================
+ Coverage   63.96%   64.07%   +0.10%     
==========================================
  Files         103      103              
  Lines       22344    22363      +19     
  Branches    10835    10851      +16     
==========================================
+ Hits        14293    14328      +35     
+ Misses       5828     5813      -15     
+ Partials     2223     2222       -1     
Files Coverage Δ
app/actions.cpp 68.55% <90.47%> (+0.92%) ⬆️

... and 5 files with indirect coverage changes

@norbertwg
Copy link
Contributor Author

This pull request is waiting for approving review since 6 months. Are there reasons not to approve this request or was it just lost out of sight?

@norbertwg norbertwg force-pushed the rename-to-allow-all-exiv2-tags branch from 28ad5f8 to 660cfcb Compare November 27, 2024 16:18
optimise regex and remove duplicate call of ImageFactory
optimise regex and remove duplicate call of ImageFactory
tests adjusted for msys and cygwin;
regex contains "Exif" for better filtering;
documentation adjusted
for concatenation of new file name, concatenation operator of std::filesystem::path is not used:
On MSYS2 UCRT64 the path separator to be used in terminal is slash, but as concatenation operator a back slash will be added. Rename works but with verbose a path with different operators will be shown.
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.

Feature Request: exiv2 rename to allow all exiv2 tags
1 participant