May 2024 - Wine-GitLab - list.winehq.org

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by Myah Caron (＠qsniyg) May 28, 2024

May 28, 2024

On Tue May 21 12:09:30 2024 +0000, eric pouech wrote: > I'd rather see this outside the do/while loop (and moreover, the second > count down shall be printed after this message) Yes, but it only overrides the "Waiting for %s seconds" part, which mostly matches Windows' behavior (from the cursor position). The only difference is that Windows will end at "Waiting for N". While it's possible to replicate this, I believe it may cause issues with translations. I can move it outside the do/while loop though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71014

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by Myah Caron (＠qsniyg) May 28, 2024

May 28, 2024

On Tue May 21 10:23:40 2024 +0000, eric pouech wrote: > why do you need float operations here? (int)(ticks_remaining / 1000) > should do It needs to be rounded because otherwise it will start with printing "Waiting for N-1 seconds" due to one or more ticks having elapsed (at least on my machine). But yes, I could do an int roundup instead (`(ticks_remaining + 500) / 1000`). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71013

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

eric pouech (@epo) commented about programs/timeout/timeout_main.c: > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <conio.h> > +#include <math.h> > + > +#include <windows.h> > + > +#include "resources.h" > + > +#include <wine/debug.h> > + > +WINE_DEFAULT_DEBUG_CHANNEL(timeout); > + > +static char* get_string(int which) a couple of remarks here (about the helpers for printing): - returning ASCII strings could lose some bits in conversion, so you'd be safer keeping unicode string - also usage of (w)printf can lack flexibility in translations (it assumes all parameters have the same order whatever the translation). FormatMessage brings more flexibility as you can change the order of parameters in output. - and likely RPRINTF_VA could be rewritten as a function and not a macro -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71012

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

I don't think this brings lots of info. Using GetStdHandle(STD_INPUT_HANDLE) throughout the code doesn't change much -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71011

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

please check GetNumberofConsoleInputEvents() return value -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71010

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

you're leaking records in case of failure -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71009

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

On Tue May 21 09:53:24 2024 +0000, eric pouech wrote: > to be pedantic, native only allows one timeout value and prints error > when two are given using strtol would be better and save you the str_is_number helper -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71008

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

eric pouech (@epo) commented about programs/timeout/timeout_main.c: > + > + do > + { > + ticks_remaining = end_ticks - GetTickCount64(); > + > + wait_millis = ticks_remaining; > + if (wait_millis > 1000) > + wait_millis = 1000; > + > + printf("\r"); > + pad_number(padded_timeout, (int)roundf(ticks_remaining / 1000.), digits); > + RPRINTF_VA(STRING_WAITING_FOR_SECONDS, padded_timeout); > + > + if (!nobreak) > + { > + if (!printed_action_key) I'd rather see this outside the do/while loop (and moreover, the second count down shall be printed after this message) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71007

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

why do you need float operations here? (int)(ticks_remaining / 1000) should do -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71006

1 0

Re: [PATCH v5 0/1] MR5691: timeout: Add new program.
by eric pouech (＠epo) May 28, 2024

May 28, 2024

actually, timeout_main.c in a bit redundant as a filename (timeout.c or main.c should be enough) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5691#note_71005

1 0