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

qrcode: fix vertical space check #2806

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

RMZeroFour
Copy link
Contributor

This PR fixes an if check in Notcurses' QR code rendering function.

This particular if statement is responsible for ensuring atleast 21 rows of space is available, the height for the smallest (version 1) QR code.

However, since the QR code is drawn with the 2x1 blitter (which maps two visual rows to a single terminal row), the effective minimum height becomes 10.5 (well, 11) rows.

Hence, QR codes that can fit in less than 21 rows (versions 1 to 5) are unnecessarily blocked by the current check.


Video demonstrating the current behaviour:
https://github.com/user-attachments/assets/e65fbed3-3f75-4150-bee6-aa4bb48d8165

Video demonstrating the fixed behaviour:
https://github.com/user-attachments/assets/55a3374c-a372-4216-b01d-1cabc31bd19b

Sample code used for the above demos:

#include <unistd.h>
#include <locale.h>

#include <notcurses/notcurses.h>

int main()
{
    setlocale(LC_ALL, "");

    notcurses_options opts{};
    notcurses* nc = notcurses_core_init(&opts, stdout);
    ncplane* stdplane = notcurses_stdplane(nc);

    char32_t key;
    do
    {
        ncplane_erase(stdplane);

        unsigned int height, width;
        ncplane_dim_yx(stdplane, &height, &width);
        
        unsigned int y = height, x = width;
        const char* text = "1234567890";
        int ver = ncplane_qrcode(stdplane, &y, &x, text, strlen(text));

        ncplane_printf_yx(stdplane, 0, 40, "VERSION: %d", ver);
        ncplane_printf_yx(stdplane, 1, 40, "SIZE: %d x %d", x, y);
        ncplane_printf_yx(stdplane, 2, 40, "PLANE: %d x %d", width, height);

    	notcurses_render(nc);
    }
    while ((key = notcurses_get_nblock(nc, nullptr)) != (char32_t)-1);

    return 0;
}

@dankamongmen
Copy link
Owner

looks good, thanks a lot!

@dankamongmen dankamongmen merged commit fc2636a into dankamongmen:master Nov 16, 2024
2 of 3 checks passed
Copy link
Owner

@dankamongmen dankamongmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrmmm, what happens if 2x1 isn't available (i.e. we're in ASCII mode)?

@dankamongmen
Copy link
Owner

hrmmm, what happens if 2x1 isn't available (i.e. we're in ASCII mode)?

in that case, ncvisual_blit() will fall back to NCBLIT_1x1 from NCBLIT_2x1, and will require the original number of rows. so to do this correctly, we need check for the availability of NCBLIT_2x1, or enforce NCBLIT_2x1 with NCVISUAL_OPTION_NODEGRADE. i'm inclined to do the latter, since only with 2x1 can you get a QR code with proper aspect ratios iirc...let me run poc/qrcode and check.

@dankamongmen
Copy link
Owner

i think i'm going to go ahead and add NCVISUAL_OPTION_NODEGRADE.

dankamongmen added a commit that referenced this pull request Nov 16, 2024
only the 2x1 blitter can generate the proper aspect ratio
needed for qrcodes, so force its use with
NCVISUAL_OPTION_NODEGRADE. see PR #2806.
dankamongmen added a commit that referenced this pull request Nov 16, 2024
only the 2x1 blitter can generate the proper aspect ratio
needed for qrcodes, so force its use with
NCVISUAL_OPTION_NODEGRADE. see PR #2806.
@RMZeroFour
Copy link
Contributor Author

hrmmm, what happens if 2x1 isn't available (i.e. we're in ASCII mode)?

Hmm, hadn't thought about that actually.

in that case, ncvisual_blit() will fall back to NCBLIT_1x1 from NCBLIT_2x1, and will require the original number of rows. so to do this correctly, we need check for the availability of NCBLIT_2x1, or enforce NCBLIT_2x1 with NCVISUAL_OPTION_NODEGRADE. i'm inclined to do the latter, since only with 2x1 can you get a QR code with proper aspect ratios iirc...let me run poc/qrcode and check.

I recompiled Notcurses to use NCBLIT_1x1 for QR codes, and while some tall stretched QR codes do seem to work depending on the scanning software, they’re inconsistent at best. Given that, and the rather wonky look, I agree that enforcing a 2x1 blitter should be fine. Most users likely don't expect tall stretched QR codes to function reliably, so that shouldn't break existing code.

@dankamongmen
Copy link
Owner

yep, i've gone ahead and implemented this requirement. thanks for the patch and causing me to look at this old-ass code. there are actually several things that ought be done to improve it, now that i look at it (for probably the first time since 2021 or so).

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.

2 participants