Bug 104045 - gfile win32 unicode fixes and refactoring
Summary: gfile win32 unicode fixes and refactoring
Status: RESOLVED MOVED
Alias: None
Product: poppler
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-03 10:12 UTC by Adrian Johnson
Modified: 2018-08-21 11:10 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Fix crash in glib demo in mingw (608 bytes, patch)
2017-12-03 10:14 UTC, Adrian Johnson
Details | Splinter Review
Enable all libraries on windows (4.51 KB, patch)
2017-12-03 10:16 UTC, Adrian Johnson
Details | Splinter Review
Remove unused cmake modules (5.93 KB, patch)
2017-12-03 10:17 UTC, Adrian Johnson
Details | Splinter Review
-DUSE_OPENJPEG2 is not used (703 bytes, patch)
2017-12-03 10:17 UTC, Adrian Johnson
Details | Splinter Review
Move UTF functions to goo (12.36 KB, patch)
2017-12-03 10:25 UTC, Adrian Johnson
Details | Splinter Review
Fix compile warning on mingw (1.30 KB, patch)
2017-12-03 10:26 UTC, Adrian Johnson
Details | Splinter Review
win32 support unicode filenames in GDir/GDirEntry (2.54 KB, patch)
2017-12-03 10:27 UTC, Adrian Johnson
Details | Splinter Review
Make GDir implementation private (4.67 KB, patch)
2017-12-03 10:28 UTC, Adrian Johnson
Details | Splinter Review
Replace UTF gmalloc/gfree with new/delete (5.85 KB, patch)
2017-12-03 10:29 UTC, Adrian Johnson
Details | Splinter Review
Replace open coded UTF8 conversion with a call to UTF.cc (2.47 KB, patch)
2017-12-03 10:30 UTC, Adrian Johnson
Details | Splinter Review
Remove unused gfile functions (7.94 KB, patch)
2017-12-03 10:31 UTC, Adrian Johnson
Details | Splinter Review
Make char* version of GooFile::open() support unicode (2.08 KB, patch)
2017-12-03 10:32 UTC, Adrian Johnson
Details | Splinter Review
Remove unused getFileNameU() function (1.74 KB, patch)
2017-12-03 10:33 UTC, Adrian Johnson
Details | Splinter Review

Description Adrian Johnson 2017-12-03 10:12:43 UTC
The aim of the following patches is to:

- Improve win32 unicode support in goo/gfile.cc.

- Eliminate duplicated functions that differ only by char*/wchar_t*. All internal functions should use char* and the win32 wchar_t conversion should be performed at the point where the win32 API is invoked.

- Eliminate unused code.

- Eventually get windows.h out of the header files. It slows down the compile and pollutes the namespace.

- Plus some other mingw fixes and build cleanups I found while working on this.
Comment 1 Adrian Johnson 2017-12-03 10:14:23 UTC
Created attachment 135889 [details] [review]
Fix crash in glib demo in mingw

Missing NULL check
Comment 2 Adrian Johnson 2017-12-03 10:16:11 UTC
Created attachment 135890 [details] [review]
Enable all libraries on windows

Some libraries are enabled, some disabled on windows which makes mingw testing harder. I see no reason why they can't all be enabled. It builds fine with cygwin/mingw.
Comment 3 Adrian Johnson 2017-12-03 10:17:22 UTC
Created attachment 135891 [details] [review]
Remove unused cmake modules
Comment 4 Adrian Johnson 2017-12-03 10:17:50 UTC
Created attachment 135892 [details] [review]
-DUSE_OPENJPEG2 is not used
Comment 5 Adrian Johnson 2017-12-03 10:25:25 UTC
Created attachment 135893 [details] [review]
Move UTF functions to goo

Move the UTF conversion functions from poppler/UTF.cc to goo/UTF.cc so they can be used in goo/gfile.cc.

In future we could potentially include UTF helper functions in GooString to simply all the code that does a two step convert the UTF then store in GooString.

The remaining code in poppler/UTF.cc has been renamed to poppler/Unicode.cc to avoid having two files with the same name.
Comment 6 Adrian Johnson 2017-12-03 10:26:47 UTC
Created attachment 135894 [details] [review]
Fix compile warning on mingw

Now that I've enabled GTK on windows, fix a compile warning in the glib demo.
Comment 7 Adrian Johnson 2017-12-03 10:27:56 UTC
Created attachment 135895 [details] [review]
win32 support unicode filenames in GDir/GDirEntry
Comment 8 Adrian Johnson 2017-12-03 10:28:57 UTC
Created attachment 135896 [details] [review]
Make GDir implementation private
Comment 9 Adrian Johnson 2017-12-03 10:29:35 UTC
Created attachment 135897 [details] [review]
Replace UTF gmalloc/gfree with new/delete
Comment 10 Adrian Johnson 2017-12-03 10:30:17 UTC
Created attachment 135898 [details] [review]
Replace open coded UTF8 conversion with a call to UTF.cc
Comment 11 Adrian Johnson 2017-12-03 10:31:27 UTC
Created attachment 135899 [details] [review]
Remove unused gfile functions

As far as I can see, these functions originated in xpdf and have never been used in poppler.
Comment 12 Adrian Johnson 2017-12-03 10:32:30 UTC
Created attachment 135900 [details] [review]
Make char* version of GooFile::open() support unicode
Comment 13 Adrian Johnson 2017-12-03 10:33:08 UTC
Created attachment 135901 [details] [review]
Remove unused getFileNameU() function
Comment 14 Adrian Johnson 2017-12-03 10:37:24 UTC
What's your opinion on supporting or removing some of the old platform macros in gfile.cc?

My understanding is MACOS is all POSIX now so the MACOS macros should not be required. I don't know if anyone uses or if it is even possible to compile poppler on ACORN or VMS, or OS/2.
Comment 15 Albert Astals Cid 2018-01-27 23:44:11 UTC
(In reply to Adrian Johnson from comment #1)
> Created attachment 135889 [details] [review] [review]
> Fix crash in glib demo in mingw
> 
> Missing NULL check

Looks good, please commit :)
Comment 16 Albert Astals Cid 2018-01-27 23:44:52 UTC
(In reply to Adrian Johnson from comment #2)
> Created attachment 135890 [details] [review] [review]
> Enable all libraries on windows
> 
> Some libraries are enabled, some disabled on windows which makes mingw
> testing harder. I see no reason why they can't all be enabled. It builds
> fine with cygwin/mingw.

I've no idea why they are disabled, i'm going to say "maybe MSVC", but since i've no clue, you can commit it and i'll blame you if someone else comes saying it broke for them :D
Comment 17 Albert Astals Cid 2018-01-27 23:45:53 UTC
(In reply to Adrian Johnson from comment #3)
> Created attachment 135891 [details] [review] [review]
> Remove unused cmake modules

Looks good, though i already removed cmake/modules/FindLIBOPENJPEG.cmake at some point, so you'll have to rebase/merge/something
Comment 18 Albert Astals Cid 2018-01-27 23:48:35 UTC
(In reply to Adrian Johnson from comment #4)
> Created attachment 135892 [details] [review] [review]
> -DUSE_OPENJPEG2 is not used

Please commit :)
Comment 19 Albert Astals Cid 2018-01-27 23:49:00 UTC
(In reply to Adrian Johnson from comment #5)
> Created attachment 135893 [details] [review] [review]
> Move UTF functions to goo
> 
> Move the UTF conversion functions from poppler/UTF.cc to goo/UTF.cc so they
> can be used in goo/gfile.cc.
> 
> In future we could potentially include UTF helper functions in GooString to
> simply all the code that does a two step convert the UTF then store in
> GooString.
> 
> The remaining code in poppler/UTF.cc has been renamed to poppler/Unicode.cc
> to avoid having two files with the same name.

I didn't check the code, but if it's just a move, sure go ahead.
Comment 20 Albert Astals Cid 2018-01-27 23:50:40 UTC
(In reply to Adrian Johnson from comment #6)
> Created attachment 135894 [details] [review] [review]
> Fix compile warning on mingw
> 
> Now that I've enabled GTK on windows, fix a compile warning in the glib demo.

The goo/glibc.h patch is "wrong", it's been fixed already in a different way.
Comment 21 Albert Astals Cid 2018-01-27 23:51:25 UTC
(In reply to Adrian Johnson from comment #7)
> Created attachment 135895 [details] [review] [review]
> win32 support unicode filenames in GDir/GDirEntry

I've no idea nor really much itnerested about caring for windows, if you think it's fine and will fix it if it's not, just commit :)
Comment 22 Albert Astals Cid 2018-01-27 23:52:27 UTC
(In reply to Adrian Johnson from comment #8)
> Created attachment 135896 [details] [review] [review]
> Make GDir implementation private

looks good :)
Comment 23 Albert Astals Cid 2018-01-27 23:54:06 UTC
(In reply to Adrian Johnson from comment #9)
> Created attachment 135897 [details] [review] [review]
> Replace UTF gmalloc/gfree with new/delete

Any particular reason for this? Also the correct way to delete a new[] is delete[] not delete.
Comment 24 Albert Astals Cid 2018-01-27 23:54:58 UTC
(In reply to Adrian Johnson from comment #10)
> Created attachment 135898 [details] [review] [review]
> Replace open coded UTF8 conversion with a call to UTF.cc

No idea of what the old code does, but less code looks good :)
Comment 25 Albert Astals Cid 2018-01-27 23:55:41 UTC
(In reply to Adrian Johnson from comment #11)
> Created attachment 135899 [details] [review] [review]
> Remove unused gfile functions
> 
> As far as I can see, these functions originated in xpdf and have never been
> used in poppler.

kill them with fire!
Comment 26 Albert Astals Cid 2018-01-27 23:57:18 UTC
(In reply to Adrian Johnson from comment #12)
> Created attachment 135900 [details] [review] [review]
> Make char* version of GooFile::open() support unicode

this only touches windows code, right? If so feel free to commit :)
Comment 27 Albert Astals Cid 2018-01-27 23:59:34 UTC
(In reply to Adrian Johnson from comment #13)
> Created attachment 135901 [details] [review] [review]
> Remove unused getFileNameU() function

Looks good.
Comment 28 Albert Astals Cid 2018-01-28 00:04:48 UTC
(In reply to Adrian Johnson from comment #14)
> What's your opinion on supporting or removing some of the old platform
> macros in gfile.cc?
> 
> My understanding is MACOS is all POSIX now so the MACOS macros should not be
> required. I don't know if anyone uses or if it is even possible to compile
> poppler on ACORN or VMS, or OS/2.

How painful is to leave them around, seem that acorn/vms and to a lesser extend os/2 are still "kind of alive", i've no idea if anyone has tried building them, but if it's not too much of a pain to keep the defines i'd let them just live.
Comment 29 GitLab Migration User 2018-08-21 11:10:58 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/poppler/poppler/issues/554.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.