-
Notifications
You must be signed in to change notification settings - Fork 282
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
Conversation
We do not want to find the package unless the PO option is enabled.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Debugging: Relative and Absolute path alternatives
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.
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:
- I was thinking you'd implement getProcessPath() by calling dumpLibraryInfo() on a
strstream
and reading the stream forexecutable=
. 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");
- 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.
Hi @clanmills , my answer to your comments between lines:
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
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
Right, I'll give another pass to the
I just removed some useless conditional sections making use of that variable:
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. |
+ Also remove inclusion of <Windows.h> from config.h
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". |
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.
Great work, Luis. Thank You.
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 toshare/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.