Ticket #49 (closed enhancement: fixed)

Opened 6 years ago

Last modified 6 years ago

Eraser 5 Unicode Support

Reported by: agent009 Owned by: Joel
Priority: major Milestone: Eraser 5.8.7
Component: Core Version: 5.8.6a
Keywords: Cc: testnutzer123@…
Processor Architecture: Blocked By:
Blocking: Operating System:

Description

Apparently eraser accesses files using ANSI functions and long filenames. This means that filenames containing characters not present in the default legacy codepage (selected in Windows international settings) cannot be accessed. The problem could be fixed by either accessing files via proper Unicode functions or by using 8.3 filenames.

Regards,

009

Blocking

IdSummaryMilestone
#49Eraser 5 Unicode SupportEraser 5.8.7

Blocked by

IdSummaryMilestone
#49Eraser 5 Unicode SupportEraser 5.8.7

Attachments

unicode.diff (240.1 KB) - added by tn123 6 years ago.
Unicode-enabling patch, v1

Change History

comment:1 Changed 6 years ago by Joel

  • Status changed from new to closed
  • Resolution set to wontfix

Yeah, I'm aware. But it's a lot of work and this is reserved for v6, which should have a public beta by end of this year (maybe even a release).

Do wait please!

Joel

comment:2 Changed 6 years ago by tn123

  • Status changed from closed to reopened
  • Resolution wontfix deleted
  • Component set to Core
  • Summary changed from International filenames to International filenames [Eraser5 Unicode support]

For what it's worth: I made a patch to enable unicode support in eraser 5.8.x (patch against trunk)

Patch is against trunk, and looks more scary than it actually is:

  • Mostly _T()
  • Some #ifdef to have ansi and unicode support code.
  • Some variable/parameter type changes (char -> TCHAR)
  • New configurations (that account for most of the patch size)


Code can still be compiled using ANSI.

========== Build: 70 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========

Dogfood'ing it since some days; no problems encountered.

Maybe it makes sense to include it in the eraser5, as eraser6 is still not really out of the door. And furthermore eraser6 will likely take some time to "settle" once released.

If you choose to accept this patch then consider the code it contains a code donation, meaning I claim no copyrights or dictate a license here.

Changed 6 years ago by tn123

Unicode-enabling patch, v1

comment:3 Changed 6 years ago by Joel

  • Status changed from reopened to accepted
  • Owner set to Joel
  • Summary changed from International filenames [Eraser5 Unicode support] to Eraser 5 Unicode Support
  • Type changed from defect to enhancement
  • Milestone set to Eraser 5.8.7

Great! I'll look into it.

comment:4 follow-up: ↓ 6 Changed 6 years ago by Joel

Hmm, a few things I've noted (mainly so I won't forget to make some minor changes when I apply the patch):

but otherwise thank you so much for the patch! It must have taken you a lot of time to slowly change those references. Good work!

comment:5 Changed 6 years ago by Joel

I'll try to implement MSLU next so Win9x users can still use the Unicode builds.

comment:6 in reply to: ↑ 4 ; follow-up: ↓ 8 Changed 6 years ago by tn123

Replying to Joel:

Hmm, a few things I've noted (mainly so I won't forget to make some minor changes when I apply the patch):

We need to write unicode filenames in unicode builds...

We need to read/write unicode.

This was a bad merge. ULONG_PTR is the right way to go, otherwise x64 may fail to populate the entropy pool from this function in some cases, I assume.

  • EraserUI/HyperLink.cpp, line 405: Why are they ifdefed out?

Because a) it's a non-standard way and basically "legacy" way. And b) the code isn't exactly compatible to newer windows versions (and IIRC there isn't even a Unicode WinExec?).

It seemed to me to be a waste of time to actually adopt the code to unicode.

  • EraserUI/ShellPidl.cpp, line 304: Is the EX structure supported under 9x?

If you trust MSDN then the answer is yes. However I assume you need to use MSLU.

huh?

Oversight again.

GetLine? sends EM_GETLINE, which specifies:

For ANSI text, this is the number of bytes; for Unicode text, this is the number of characters.

sizeof(TCHAR[8]) is 8 in ansi mode (TCHAR being char) but 16 in unicode (TCHAR being wchar_t), so it would be incorrect in unicode.
Another correct version would be the following, but I find it hard to understand:

if (m_eKey.GetLine(0, ch, sizeof(ch) / sizeof(TCHAR)))

Same as with KeyComboDlg?.. msdn states:

The size of the lpFilename buffer, in TCHARs.

not size in bytes.

but otherwise thank you so much for the patch! It must have taken you a lot of time to slowly change those references. Good work!

It didn't actually take to long. ;)

comment:7 Changed 6 years ago by tn123

  • Cc testnutzer123@… added

comment:8 in reply to: ↑ 6 Changed 6 years ago by Joel

Heh, for most part of the comment it was basically stuff I'd wanna review again after merging the patch since the contextual meaning was kinda lost when going through the diff file alone.

Replying to tn123:

Replying to Joel:

Hmm, a few things I've noted (mainly so I won't forget to make some minor changes when I apply the patch):

We need to write unicode filenames in unicode builds...

Hah! Consequence of using C# for too long, I assure you that your brain wastes away when you use C#! Too pampered...

We need to read/write unicode.

Actually since RegQueryValue? takes an LPBYTE it doesn't matter how we declared the buffer, but yes I get the code flow after reading the merged sources

This was a bad merge. ULONG_PTR is the right way to go, otherwise x64 may fail to populate the entropy pool from this function in some cases, I assume.

Yep, I fixed that

  • EraserUI/HyperLink.cpp, line 405: Why are they ifdefed out?

Because a) it's a non-standard way and basically "legacy" way. And b) the code isn't exactly compatible to newer windows versions (and IIRC there isn't even a Unicode WinExec?).

It seemed to me to be a waste of time to actually adopt the code to unicode.

I updated it with a CreateProcess? version. Now you know why I say that v5 is getting more and more expensive to fix and maintain, ha.

  • EraserUI/ShellPidl.cpp, line 304: Is the EX structure supported under 9x?

If you trust MSDN then the answer is yes. However I assume you need to use MSLU.

Probably; until I ran the binaries in 9x and found that VS2008 doesn't create binaries for 9x and NT anymore.

huh?

Oversight again.

GetLine? sends EM_GETLINE, which specifies:

For ANSI text, this is the number of bytes; for Unicode text, this is the number of characters.

sizeof(TCHAR[8]) is 8 in ansi mode (TCHAR being char) but 16 in unicode (TCHAR being wchar_t), so it would be incorrect in unicode.
Another correct version would be the following, but I find it hard to understand:

if (m_eKey.GetLine(0, ch, sizeof(ch) / sizeof(TCHAR)))

Same as with KeyComboDlg?.. msdn states:

The size of the lpFilename buffer, in TCHARs.

not size in bytes.

Yep, I was kinda looking for that since that's how I usually pass such values to WinAPI (sorry about "nitpicking" coz we kinda need to maintain some code standards here)

but otherwise thank you so much for the patch! It must have taken you a lot of time to slowly change those references. Good work!

It didn't actually take to long. ;)

Thanks anyway!

comment:9 Changed 6 years ago by Joel

  • Status changed from accepted to closed
  • Resolution set to fixed

Regarding MSLU once more - I don't see the point in doing so since our binaries will no longer work with 9x (VS2008 target platform support removed)

9x users will probably have to use 5.7.

comment:10 Changed 6 years ago by tn123

Just a reminder:
The setup (eraser.iss) wasn't changed to modify the unicode-enabled builds (simply because I was to lazy to set up the innosetup stack). You'll need to adjust these as well.

comment:11 Changed 6 years ago by Joel

Okay I have, thanks for letting me know. v5 is gold!

Note: See TracTickets for help on using tickets.