gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows #13734

pull ken2812221 wants to merge 2 commits into bitcoin:master from ken2812221:drop-boost-scoped-array changing 3 files +10 −28
  1. ken2812221 commented at 10:24 pm on July 21, 2018: contributor

    Drop boost::scoped_array and simplify the code.

    TCHAR should be defined as wchar_t if UNICODE is defined. So we can use .toStdWString().c_str() to get wchar_t C-style string.

    Fix #13819

  2. MarcoFalke added the label GUI on Jul 22, 2018
  3. MarcoFalke renamed this:
    Drop boost::scoped_array
    gui: Drop boost::scoped_array
    on Jul 22, 2018
  4. fanquake added this to the "In progress" column in a project
  5. fanquake commented at 4:08 am on July 22, 2018: member
    This was introduced in #5793. Looks like something similar to what you are doing was proposed at the time as well.
  6. ken2812221 commented at 4:47 am on July 22, 2018: contributor
    Hope there is a way to see code that is before he force push.
  7. practicalswift commented at 5:42 am on July 22, 2018: contributor

    Concept ACK

    Thanks for removing Boost dependencies. Keep it coming! :-)

  8. MarcoFalke commented at 12:37 pm on July 22, 2018: member
    Any suggestion on how to test this?
  9. ken2812221 commented at 1:50 pm on July 22, 2018: contributor
    This is Windows-only code, can be tested as same as #5793 (comment)
  10. MarcoFalke added the label Needs gitian build on Jul 22, 2018
  11. DrahtBot removed the label Needs gitian build on Jul 22, 2018
  12. in src/qt/guiutil.cpp:617 in 06430539fb outdated 
    614@@ -625,7 +615,7 @@ bool SetStartOnSystemStartup(bool fAutoStart)
615 #ifndef UNICODE
616             psl->SetArguments(strArgs.toStdString().c_str());
617 #else
    Sjors commented at 3:35 pm on July 24, 2018:
    Add a comment that UNICODE is only defined for Windows? (if that’s the case)
  13. Sjors commented at 3:49 pm on July 24, 2018: member

    Should the reference also be removed from test/lint/lint-includes.sh?

    Tested that make still works on macOS.

    Tested the gitian binary on a Windows 10 VM, by launching with bitcoin-qt -wallet=你好, assuming that was the problem? This leads to a crash, but that’s also the case on master:

    I have a version from earlier this year where it leads to a slightly nicer crash:

    (seems unrelated, so I made a ticket #13754)

    bitcoin-qt -wallet=test works fine by the way, maybe that was all.

  14. ken2812221 force-pushed on Jul 29, 2018
  15. ken2812221 commented at 11:19 am on July 29, 2018: contributor
    Update: Since the goal is to make it unicode compatible, just make it use wchar_t version of api explicitly.
  16. ken2812221 renamed this:
    gui: Drop boost::scoped_array
    gui: Drop boost::scoped_array and use wchar_t API explicitly on Windows
    on Jul 29, 2018
  17. in src/qt/guiutil.cpp:559 in 5d70ab06a1 outdated 
    554 
555         if (SUCCEEDED(hres))
556         {
557             // Get the current executable path
558-            TCHAR pszExePath[MAX_PATH];
559-            GetModuleFileName(nullptr, pszExePath, sizeof(pszExePath));
    ken2812221 commented at 11:25 am on July 29, 2018:
    This is bug before, if TCHAR is wchar_t, then sizeof(pszExePath)= 2*MAX_PATH, which can result memory overflow.
  18. MarcoFalke deleted a comment on Jul 29, 2018
  19. MarcoFalke added the label Windows on Jul 29, 2018
  20. MarcoFalke added the label Needs gitian build on Jul 29, 2018
  21. DrahtBot removed the label Needs gitian build on Jul 29, 2018
  22. Sjors commented at 1:23 pm on July 30, 2018: member
    Tested that make still works on macOS with 5d70ab0. I have no opinion on the code itself, other than it seems good to keep making progress here: https://github.com/bitcoin/bitcoin/projects/3#card-205363
  23. MarcoFalke commented at 2:15 pm on July 31, 2018: member

    Tested that for commit 6031f7f (master and this pull) I can launch regtest and testnet after re-login for a user with only ascii character in the name.

    (See lower left of screenshot, Compare to #5793 (comment))

    screenshot from 2018-07-31 10-13-36

  24. MarcoFalke requested review from jonasschnelli on Jul 31, 2018
  25. ken2812221 force-pushed on Jul 31, 2018
  26. ken2812221 commented at 3:20 pm on July 31, 2018: contributor
    @MarcoFalke Can you test this again? Now it should fix #13819.
  27. MarcoFalke added the label Needs gitian build on Jul 31, 2018
  28. MarcoFalke added this to the milestone 0.17.0 on Jul 31, 2018
  29. DrahtBot removed the label Needs gitian build on Aug 1, 2018
  30. MarcoFalke commented at 3:36 pm on August 1, 2018: member

    Now it fails for commit d41914b (master and this pull) that it can not write to the directory:

    (though the folders are created, as can be seen in the explorer in the background)

    screenshot from 2018-08-01 11-35-25

  31. MarcoFalke removed this from the milestone 0.17.0 on Aug 1, 2018
  32. MarcoFalke added this to the milestone 0.18.0 on Aug 1, 2018
  33. ken2812221 commented at 3:52 pm on August 1, 2018: contributor
    @MarcoFalke That should fixed by #13426. You can merge these two PR and test locally.
  34. in src/qt/guiutil.cpp:556 in cefd242d60 outdated 
    555         if (SUCCEEDED(hres))
556         {
557             // Get the current executable path
558-            TCHAR pszExePath[MAX_PATH];
559-            GetModuleFileName(nullptr, pszExePath, sizeof(pszExePath));
560+            wchar_t pszExePath[MAX_PATH];
    Empact commented at 4:22 am on August 3, 2018:
    I think safer to use WCHAR here, as I’m seeing differing descriptions of its definition, e.g. unsigned wchar_t here: https://docs.microsoft.com/en-us/windows/desktop/intl/windows-data-types-for-strings
    ken2812221 commented at 5:16 pm on August 3, 2018:
    fixed
  35. in src/util.cpp:1121 in cefd242d60 outdated 
    1117@@ -1118,9 +1118,9 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length) {
1118 #ifdef WIN32
1119 fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
1120 {
1121-    char pszPath[MAX_PATH] = "";
1122+    wchar_t pszPath[MAX_PATH] = L"";
    Empact commented at 4:23 am on August 3, 2018:
    Same, WCHAR seems preferable.
    ken2812221 commented at 5:16 pm on August 3, 2018:
    fixed
  36. DrahtBot commented at 4:59 pm on August 3, 2018: member
  37. Drop boost::scoped_array 1c5d225853
  38. ken2812221 force-pushed on Aug 3, 2018
  39. in src/util.cpp:1123 in b446918fc6 outdated 
    1117@@ -1118,9 +1118,9 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length) {
1118 #ifdef WIN32
1119 fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
1120 {
1121-    char pszPath[MAX_PATH] = "";
1122+    WCHAR pszPath[MAX_PATH] = L"";
1123 
1124-    if(SHGetSpecialFolderPathA(nullptr, pszPath, nFolder, fCreate))
1125+    if(SHGetSpecialFolderPathW(nullptr, pszPath, nFolder, fCreate))
    Empact commented at 6:37 pm on August 3, 2018:
    nit: the failure message below references SHGetSpecialFolderPathA
    ken2812221 commented at 6:48 pm on August 3, 2018:
    fixed
  40. Empact commented at 6:37 pm on August 3, 2018: member
    utACK b446918
  41. gui: get special folder in unicode bb6ca65f98
  42. ken2812221 force-pushed on Aug 3, 2018
  43. ken2812221 closed this on Aug 21, 2018
  44. ken2812221 reopened this on Aug 21, 2018
  45. MarcoFalke deleted a comment on Aug 21, 2018
  46. MarcoFalke deleted a comment on Aug 21, 2018
  47. laanwj commented at 8:57 am on September 11, 2018: member

    nice cleanup, too

    utACK bb6ca65f9890e8280ace32de5a37774e14705859

  48. laanwj merged this on Sep 11, 2018
  49. laanwj closed this on Sep 11, 2018
  50. laanwj referenced this in commit 362518791a on Sep 11, 2018
  51. ken2812221 deleted the branch on Sep 11, 2018
  52. fanquake moved this from the "In progress" to the "Done" column in a project
  53. Warrows referenced this in commit 34bbaeb75f on Oct 14, 2019
  54. Warrows referenced this in commit 542a1db4de on Nov 23, 2019
  55. deadalnix referenced this in commit c61d5a151d on Apr 29, 2020
  56. ftrader referenced this in commit fb95042820 on Aug 17, 2020
  57. zkbot referenced this in commit 77c2a5f810 on Dec 4, 2020
  58. random-zebra referenced this in commit f15a1674ce on Feb 6, 2021
  59. Munkybooty referenced this in commit 60ff2a65b3 on Jul 7, 2021
  60. Munkybooty referenced this in commit 6537edc78c on Jul 8, 2021
  61. Munkybooty referenced this in commit 223a358002 on Jul 9, 2021
  62. Munkybooty referenced this in commit df4af75a8c on Jul 11, 2021
  63. Munkybooty referenced this in commit d45b55dca5 on Jul 12, 2021
  64. Munkybooty referenced this in commit 3f79d489d1 on Jul 12, 2021
  65. Munkybooty referenced this in commit 57c7050f4d on Jul 12, 2021
  66. Munkybooty referenced this in commit c585c4a792 on Jul 13, 2021
  67. kittywhiskers referenced this in commit 000fdcb6bc on Jul 13, 2021
  68. kittywhiskers referenced this in commit 98ae047fdf on Jul 13, 2021
  69. kittywhiskers referenced this in commit 0e863d1dee on Jul 13, 2021
  70. random-zebra referenced this in commit 61a098a775 on Aug 5, 2021
  71. DrahtBot locked this on Sep 8, 2021


ken2812221 fanquake practicalswift MarcoFalke Sjors Empact DrahtBot laanwj


jonasschnelli

Labels
GUI Windows

Milestone
0.18.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me