View Issue Details

IDProjectCategoryView StatusLast Update
0001027OpenMPT[All Projects] Generalpublic2018-02-23 12:14
ReportermanxAssigned Tomanx 
PrioritylowSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
Product VersionOpenMPT 1.27.01.00 / libopenmpt 0.3.1 (upgrade first) 
Target VersionOpenMPT 1.28.01.00 (goals)Fixed in Version 
Summary0001027: Remove various WCHAR hacks for !UNICODE
Description

There are various hacks in the codebase that were only needed for unicode filename support in !unicode builds. Those can be removed now, as well as all explicit call too FooW WinAPI functions.
We should also consider removing all wide filename support in !unicode builds completely. This could simplify ansi builds which I still think would be useful to continue supporting as they tend to show inconsistent string type uses.

Additional Information

big areas:

  • filename UTF8 tunneling in document handling
  • CTreeCtrl.h
  • maybe FileDialog
  • maybe even TCHAR mpt::PathString (probably not though)
TagsNo tags attached.
Has the bug occurred in previous versions?
Tested code revision (in case you know it)

Relationships

related to 0000570 resolvedmanx OpenMPT UNICODE build 

Activities

manx

manx

2017-10-28 08:22

administrator   ~0003316

r9154 removes filename UTF8 tunneling in document handling

Saga Musix

Saga Musix

2018-02-11 22:55

administrator   ~0003415

Last edited: 2018-02-11 22:56

View 2 revisions

We need to decide if we want TCHAR PathString or not; if we don't, modifying CTreeCtrl.h doesn't make much sense, as it's used in PathString-related contexts only. So it's either both or none.
For the same reason I'd also not modify the file dialogs if PathString stays WCHAR.

manx

manx

2018-02-12 08:23

administrator   ~0003416

Agreed.
I tried modifying CTreeCtrl (and related) once, and it resulted in a rather huge mess if PathString stays WCHAR.
IF when make PathString TCHAR, we would probably have to modify various places that currently assume PathString::AsNative() is WCHAR-based. This would either be a huge single commit, changing all usage sites, or would gradually introduce #ifdef around these assumption which could again be removed when we actually do the switch of PathString to TCHAR. Both options appear like a lot of work to me.

manx

manx

2018-02-12 12:51

administrator   ~0003417

Another option might of course be to just completely remove support for ANSI builds. This would cost us some compile-time string type checking though.

Saga Musix

Saga Musix

2018-02-12 16:25

administrator   ~0003418

Removing explicit WCHAR support in FileDIalog would allow us to use CFileDialog again, which has the advantage of automatically using IFileDialog when available rather than the classic GetOpenFileName/GetSaveFileName. This in return has the advantage that specifying a hook procedure for selection changes (like we do when "preview samples in file dialogs" is checked) does not enforce the "classic" pre-Vista file dialogs.

manx

manx

2018-02-23 11:42

administrator   ~0003434

PathString is TCHAR based as of r9657 .
FileDialog uses CFileDialog again as of r9670 through r9692 (Saga Musix).
CTreeCtrl.h is gone as of r9746 (Saga Musix).

manx

manx

2018-02-23 11:44

administrator   ~0003435

COM GUID functionality is still std::wstring, however the COM API is based on OLECHAR, which is WCHAR even in ANSI builds, thus it does not make much sense to change that area.

manx

manx

2018-02-23 11:45

administrator   ~0003436

Are there any other bigger areas that still need to get converted?

Saga Musix

Saga Musix

2018-02-23 11:49

administrator   ~0003437

Any usage of mpt::ustring in the UI leads to using the widechar WinAPI functions, but I don't think that's necessarily an issue that needs to be fixed. The only time this is going to be relevant will be if we switch to a GUI toolkit that prefers utf8 strings over wide strings, in which case we have to remove the WinAPI functions that are being called anyway. So I'd leave these sites as-is and consider this issue to be resolved.

manx

manx

2018-02-23 12:14

administrator   ~0003438

There are probably still some minor places that could use cleanup.
However, not even searching for "W(" does show anything big at all by now.
PluginBridge is still mostly WCHAR based, I do not think it makes much sense to change that though.
Closing the issue.

Issue History

Date Modified Username Field Change
2017-09-21 12:28 manx New Issue
2017-09-21 12:28 manx Status new => assigned
2017-09-21 12:28 manx Assigned To => manx
2017-09-21 12:29 manx Relationship added related to 0000570
2017-10-28 08:20 manx Additional Information Updated View Revisions
2017-10-28 08:21 manx Additional Information Updated View Revisions
2017-10-28 08:22 manx Note Added: 0003316
2017-10-28 08:22 manx Additional Information Updated View Revisions
2018-02-11 22:55 Saga Musix Note Added: 0003415
2018-02-11 22:56 Saga Musix Note Edited: 0003415 View Revisions
2018-02-12 08:23 manx Note Added: 0003416
2018-02-12 12:51 manx Note Added: 0003417
2018-02-12 16:25 Saga Musix Note Added: 0003418
2018-02-16 21:37 Saga Musix Additional Information Updated View Revisions
2018-02-23 11:42 manx Note Added: 0003434
2018-02-23 11:43 manx Additional Information Updated View Revisions
2018-02-23 11:44 manx Note Added: 0003435
2018-02-23 11:45 manx Note Added: 0003436
2018-02-23 11:49 Saga Musix Note Added: 0003437
2018-02-23 12:14 manx Status assigned => resolved
2018-02-23 12:14 manx Resolution open => fixed
2018-02-23 12:14 manx Note Added: 0003438