Skip to content
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 broken doskey macro functionality with Windows 10 (15002)'s modern cmd #464

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qwerty12
Copy link

@qwerty12 qwerty12 commented Jul 13, 2017

For #438. As GetConsoleAlias works on the byte count, and not the character count, using 1 for the buffer size will fail some TargetBufferLength < sizeof(WCHAR) test I think (read: imagine) the more stringent Conhost V2 has somewhere and will always return 0 instead of succeeding like it does with V1, causing clink to think no doskey aliases exist.
By using the correct size, the GetConsoleAliasW call will still fail (in most cases - not sure if GCA will refuse to write to the buffer if there isn't space for a null terminator), but the last error code will now reflect if it did so because it actually couldn't find the alias, or if the buffer size was too small.

@qwerty12 qwerty12 changed the title Fix broken doskey alias functionality with Windows 10 (15002)'s modern cmd Fix broken doskey macro functionality with Windows 10 (15002)'s modern cmd Jul 13, 2017
@vvs
Copy link

vvs commented Jul 16, 2017

Thank you! I can confirm that with these changes the doskey macros are back! At least on my Windows 10 x64 Creators Update.

@ghost
Copy link

ghost commented Jul 21, 2017

Any kind soul care to share a build for those of us who have no ability (or idea how) to build this thing?

@Grzegorz-Abram
Copy link

Compiled master + changed 'clink/dll/doskey.c'
https://drive.google.com/open?id=0B9NoUudepr_uRjl1Q01sVHlaOW8

@daxgames
Copy link

daxgames commented Aug 2, 2017

@mridgers bump. Would be nice to get this merged if it does fix the issue. Cmder #1374 is related to this pr and a fix would be great. Thanks in advance!

@twig
Copy link

twig commented Aug 4, 2017

using the binaries from @Grzegorz-Abram on ConEmu works nicely, but causes issues when trying to use ctrl+R for recent history filtering (noticeably when typing S key)

@vvs
Copy link

vvs commented Aug 4, 2017

@twig What kind of issues, could you elaborate? So I could double-check :)

@twig
Copy link

twig commented Aug 4, 2017

Not sure where the error is ocurring exactly but here's the specifics.

  • ConEmu v170402
  • clink build from earlier in this issue
  • Windows 10 x64 (with creators update)

Scenario: I type "tas" into reverse search and it works

Video 1: working properly with ConEmu's bundled clink

Video 2: with custom build clink

Notice how the focus is shifted away from the ConEmu window as I press the "s" in "tas".

@vvs
Copy link

vvs commented Aug 5, 2017

@twig, thanks. Not reproducible on my end, also with ConEmu (170605) and the updated clink.

@twig
Copy link

twig commented Aug 7, 2017

was your clink installed via setup or portable?

@ankostis
Copy link

ankostis commented Aug 7, 2017

Thank you @Grzegorz-Abram!

I noticed that the new executable x5 bigger than the old one.
Do you happen to know the reason?

@Grzegorz-Abram
Copy link

Grzegorz-Abram commented Aug 7, 2017

@ankostis I don't know 😄 I have only pre-basic experience with C language 😄
I've just cloned the repo, cherry-picked file from this PR and run these commands:

premake.exe gmake
cd .build\gmake
mingw32-make.exe config=release_x64 all

I suppose, that there were missing some compilation flags or something like that 😄 I've left that optimisation magic for real C wizards 😄

@ankostis
Copy link

ankostis commented Aug 7, 2017

(noting that the DLL has retained its size)

@vvs
Copy link

vvs commented Aug 7, 2017

@Grzegorz-Abram You could also 'strip' the binaries to further reduce their size. So the DLL + EXE will become much smaller (400kb total vs almost 4MB) 8-)

@Grzegorz-Abram
Copy link

Ok, I've done it! I hope so... 😄 BTW, I had one false alarm - Avast claimed that binaries contains something dangerous... Maybe it doesn't like stripped binaries... 😄

@brownbl1
Copy link

brownbl1 commented Aug 7, 2017

@Grzegorz-Abram Works for me! And no virus warnings on my machine. :)

Thanks!

ankostis added a commit to JRCSTU/allinone that referenced this pull request Aug 7, 2017
@twig
Copy link

twig commented Aug 7, 2017

Oddly enough, these work perfectly fine for me

Thanks!

@Stanzilla
Copy link

I've setup a fork that has some PRs merged and generates builds via appveyor: https://github.com/Stanzilla/clink/commits/master

The zips it generates contain a bit more than the usual release zips from @mridgers but I just threw something together real quick. Any help making this better would be appreciated.

@KayLeung
Copy link

I cannot find the zip from @Stanzilla, so I build it my own. Here's a VS2015 build:
https://drive.google.com/drive/folders/0B2Eb5FVA34XTcXBvRlRFQ1R3RFU

@Grzegorz-Abram
Copy link

@Stanzilla It's not that I'm complaining, I'm really appreciate your work. But... Would it be possible to configure AppVeyor to build both x86 and x64 releases?

@Stanzilla
Copy link

Stanzilla commented Aug 16, 2017

yeah probably, just have to figure out how, I never did anything with premake before.

@KayLeung
Copy link

@Stanzilla try premake5.exe clink_release

@AcousticRand
Copy link

@KayLeung - thank you for compiling and posting those binaries to your google drive. I put them in place and everything's working great without legacy mode turned on! I appreciate it!

@jrappen
Copy link

jrappen commented Oct 15, 2017

@mridgers Any reasons this isn't merged?

@garoto
Copy link

garoto commented Oct 16, 2017

mridgers is AWOL for a while now.

@mattdkerr
Copy link

Confirmed working with final build of Fall Creator's Update with modern console.

@jrappen
Copy link

jrappen commented Oct 23, 2017

Whoever is interested in this PR, it was merged and is included in the recent build at https://github.com/Stanzilla/clink/releases


Also, for cmderdev/cmder users: The link there was redirected to use @Stanzilla's fork, the next build should include this.

@ankostis
Copy link

@jrappen so are you forking the project and taking over?
Better then mark this fact on your readme?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.