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

Fix localisation issues #535

Merged
merged 11 commits into from
Nov 11, 2018
Merged

Fix localisation issues #535

merged 11 commits into from
Nov 11, 2018

Conversation

piponazo
Copy link
Collaborator

@piponazo piponazo commented Nov 9, 2018

This PR is fixing some of the problems we had with the localisation system (#527).

The main problem was the EXV_ENABLE_NLS was never defined and therefore all code related with NLS was never exercised.

I also found that the value we were using for EXV_LOCALEDIR was not correct. I added a new function (getProcessPath()) to obtain the path where the executable is. From that path we can then compose at runtime the path where the Exiv2 .MO files should be found. Note that we use the CMake variable ${CMAKE_INSTALL_LOCALEDIR} from the GNUInstallDirs CMake Module (which normally points to share/locale).

This should also works on Windows, but we are not handling the libintl dependency with conan yet. This would be something to investigate in the future.

We do not want to find the package unless the PO option is enabled.
@piponazo piponazo added this to the v0.27 milestone Nov 9, 2018
@piponazo piponazo self-assigned this Nov 9, 2018
@codecov
Copy link

codecov bot commented Nov 9, 2018

Codecov Report

Merging #535 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
+ Coverage    63.6%   63.64%   +0.04%     
==========================================
  Files         154      154              
  Lines       20575    20596      +21     
==========================================
+ Hits        13087    13109      +22     
+ Misses       7488     7487       -1
Impacted Files Coverage Δ
src/value.cpp 79.18% <ø> (-0.04%) ⬇️
include/exiv2/types.hpp 96.15% <ø> (ø) ⬆️
include/exiv2/futils.hpp 100% <ø> (ø) ⬆️
src/image_int.cpp 39.13% <ø> (ø) ⬆️
src/version.cpp 92.76% <ø> (ø) ⬆️
src/http.cpp 0% <ø> (ø) ⬆️
src/exiv2app.hpp 87.5% <ø> (ø) ⬆️
src/exiv2.cpp 67.53% <100%> (+0.19%) ⬆️
src/types.cpp 96.13% <100%> (+0.07%) ⬆️
unitTests/test_futils.cpp 100% <100%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 274ef04...980b2ea. Read the comment docs.

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

Very good and quick work @piponazo Thanks very much.

I'd very much like to pull down and build this code. However, as I'm on vacation, I don't have time.

I'd like to make a couple of comments and then approve this on Saturday morning.

getProcessPath() is a good idea. Two thoughts:

  1. I was thinking you'd implement getProcessPath() by calling dumpLibraryInfo() on a strstream and reading the stream for executable=. This would eliminate the need to duplicate calls to GetProcessModules() etc.

There could be an API std::string getLIbraryInfo(const std::string& key) to search for anything in dumpLibraryInfo() without duplicating the low-level code to discover that information and the implementation of getProcessPath() would be return getLibraryInfo("executable");

  1. Can you eliminate code in version.cpp to set executable=getProcesPath();

I'm delight that you seem to have discovered a way to eliminate some of the EXV_HAVE_DLL and EXV_EXPORT. I see there's code in include/exiv2/config.h concerning MSVC 2003 which I be safely removed. I suspect the references to 2005 can be increased to 2008 as the most elderly supported edition.

I see you've changed some CMake if ( BUILD_SHARED_LIBS ) to if ( ${BUILD_SHARED_LIBS} ) is this because we're overloading BUILD_SHARED_LIBS`? Perhaps we should have an option EXIV2_BUILD_SHARED and we respect CMake's variable BUILD_SHARED_LIBS. Or perhaps I've misunderstood.

Anyway. Great Work @piponazo I'm interested to hear your comments then we'll get this approved and into the system and possibly integrated with 0.27RC2.

@piponazo
Copy link
Collaborator Author

Hi @clanmills , my answer to your comments between lines:

  1. I was thinking you'd implement getProcessPath() by calling dumpLibraryInfo() on a strstream and reading the stream for executable=. This would eliminate the need to duplicate calls to GetProcessModules() etc.

I considered to do something like this but the I thought: "I just want to get the process path as quickly as possible". By calling dumpLibraryInfo() we would be doing much more useless work for just obtaining the path to the executable.

  1. Can you eliminate code in version.cpp to set executable=getProcesPath();

This would be possible, but we would only be saving few lines of code for the Linux case. On Windows and Mac the code of dumpLibraryInfo() would still be the same.

I'm delight that you seem to have discovered a way to eliminate some of the EXV_HAVE_DLL and EXV_EXPORT. I see there's code in include/exiv2/config.h concerning MSVC 2003 which I be safely removed. I suspect the references to 2005 can be increased to 2008 as the most elderly supported edition.

Right, I'll give another pass to the config.h file to remove deprecated stuff.

I see you've changed some CMake if ( BUILD_SHARED_LIBS ) to if ( ${BUILD_SHARED_LIBS} ) is this because we're overloading BUILD_SHARED_LIBS`? Perhaps we should have an option EXIV2_BUILD_SHARED and we respect CMake's variable BUILD_SHARED_LIBS. Or perhaps I've misunderstood.

I just removed some useless conditional sections making use of that variable:

if ( BUILD_SHARED_LIBS )	
        # something
endif()

But I did not do any replacement.

As a final comment, I would not like to do deeper changes related to the localisation just before releasing 0.27. In this PR my main objective was to fix the situation with the CMake - Intl interaction so that exiv2 could use the translations easily. I would left any additional work regarding this topic for future releases.

@clanmills
Copy link
Collaborator

This is really good and I'm happy to approve this. I can't see the "Review" button. Did @D4N approve this? I think we're going to deal with localisation in two "phases".
RC2: (2018-11-16) Working, documented and in the build bundles.
RC3: (2018-12-07) Working as you intend. Code complete.
--- Code Freeze ---
GM: (2018-12-28) I hope we have no code changes after RC3

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

Great work, Luis. Thank You.

@clanmills clanmills removed this from the v0.27 milestone Nov 11, 2018
@piponazo piponazo merged commit aae84e4 into Exiv2:master Nov 11, 2018
@piponazo piponazo deleted the translations branch November 11, 2018 11:27
@clanmills clanmills mentioned this pull request Nov 15, 2018
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