DanBloomberg / leptonica

Leptonica is an open source library containing software that is broadly useful for image processing and image analysis applications. The official github repository for Leptonica is: danbloomberg/leptonica. See leptonica.org for more documentation.
Other
1.74k stars 387 forks source link

`pixDisplayWithTitle()`: supports 'open' mode on Win32/64 + supports 'store' mode for all #683

Closed GerHobbelt closed 1 year ago

GerHobbelt commented 1 year ago

pixDisplayWithTitle():

GerHobbelt commented 1 year ago

😢 NONE mode is currently equiv. to STORE mode. fix commit will be added to his pullreq. 😊

DanBloomberg commented 1 year ago

I don't see the point in most of this.

We have a global L_DISPLAYWITH... that can be set with l_chooseDisplayProg(), but for most situations the default suffices. If you don't want output, use 0 for the dispflag in pixDisplayWithTitle -- we don't need to call l_chooseDisplayProg(L_DISPLAY_WITH_NONE) first. If you want to write the image to file, use pixWrite() -- there's no good way to do it from pixDisplay(), and your code doesn't do it either. There is no place to specify the output filename.

Did you mean to have the default windows display program be "open"? If so, why that and why not

For windows, what is wrong with getting the path on the stack with:

    pathname = genPathname(tempname, NULL);
    _fullpath(fullpath, pathname, sizeof(fullpath));

?

For Windows, we can allow use of the 'open' display program with this variant of your code:

        /* Windows: L_DISPLAY_WITH_IV or L_DISPLAY_WITH_OPEN */
    pathname = genPathname(tempname, NULL);
    _fullpath(fullpath, pathname, sizeof(fullpath));
    if (var_DISPLAY_PROG == L_DISPLAY_WITH_IV) {
        if (title) {
            snprintf(buffer, Bufsize,
                     "i_view32.exe \"%s\" /pos=(%d,%d) /title=\"%s\"",
                     fullpath, x, y, title);
        } else {
            snprintf(buffer, Bufsize, "i_view32.exe \"%s\" /pos=(%d,%d)",
                     fullpath, x, y);
        }
    } else {  /* L_DISPLAY_WITH_OPEN */
        snprintf(buffer, Bufsize, "explorer.exe /open \"%s\"", fulllpath);
    }

Interesting syntax with the comma in the command line: explorer.exe /open,\"%s\"

I'll add checks at the top of the function to make sure that the display program is valid for the platform.

GerHobbelt commented 1 year ago

Okay on the first part.

Re explorer.exe /open,"%s": that is not an open application: it's telling MSWindows to open the application that's currently registered as the 'default viewer' for the given file type.

Technically, it calls Windows Explorer (explorer.exe, which is part of MSwindows) to find application associated with the 'open' action for the given file type of file %1 -- the association is based on the file extension. This is the usual approach to open a file/image viewer for a Windows user: it opens the system default viewer unless the user has previously configured another viewer instead, e.g. ACDSee, irfanview, ImageGlass, ......

Met vriendelijke groeten / Best regards,

Ger Hobbelt


web: http://www.hobbelt.com/ http://www.hebbut.net/ mail: @.*** mobile: +31-6-11 120 978

On Mon, Mar 20, 2023 at 10:04 PM Dan Bloomberg @.***> wrote:

I don't see the point in most of this.

We have a global L_DISPLAYWITH... that can be set with l_chooseDisplayProg(), but for most situations the default suffices. If you don't want output, use 0 for the dispflag in pixDisplayWithTitle -- we don't need to call l_chooseDisplayProg(L_DISPLAY_WITH_NONE) first. If you want to write the image to file, use pixWrite() -- there's no good way to do it from pixDisplay(), and your code doesn't do it either. There is no place to specify the output filename.

Did you mean to have the default windows display program be "open"? If so, why that and why not

For windows, what is wrong with getting the path on the stack with:

pathname = genPathname(tempname, NULL); _fullpath(fullpath, pathname, sizeof(fullpath));

?

For Windows, we can allow use of the 'open' display program with this variant of your code:

    /* Windows: L_DISPLAY_WITH_IV or L_DISPLAY_WITH_OPEN */
pathname = genPathname(tempname, NULL);
_fullpath(fullpath, pathname, sizeof(fullpath));
if (var_DISPLAY_PROG == L_DISPLAY_WITH_IV) {
    if (title) {
        snprintf(buffer, Bufsize,
                 "i_view32.exe \"%s\" /pos=(%d,%d) /title=\"%s\"",
                 fullpath, x, y, title);
    } else {
        snprintf(buffer, Bufsize, "i_view32.exe \"%s\" /pos=(%d,%d)",
                 fullpath, x, y);
    }
} else {  /* L_DISPLAY_WITH_OPEN */
    snprintf(buffer, Bufsize, "explorer.exe /open \"%s\"", fulllpath);
}

Interesting syntax with the comma in the command line: explorer.exe /open,"%s"

I'll add checks at the top of the function to make sure that the display program is valid for the platform.

— Reply to this email directly, view it on GitHub https://github.com/DanBloomberg/leptonica/pull/683#issuecomment-1476931973, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADCIHUHYJR63W6OREHNZSLW5DA5JANCNFSM6AAAAAAWBOT5RI . You are receiving this because you authored the thread.Message ID: @.***>

GerHobbelt commented 1 year ago

BTW:

If you don't want output, use 0 for the dispflag in pixDisplayWithTitle

yep, I noticed, but that's not going to fly for pixDisplay() calls, such as in the /prog/*.c files, because that one hardcodes display=1 as part of the arguments it passes to pixdisplayWithTitle() under the hood.

Met vriendelijke groeten / Best regards,

Ger Hobbelt


web: http://www.hobbelt.com/ http://www.hebbut.net/ mail: @.*** mobile: +31-6-11 120 978

On Mon, Mar 20, 2023 at 10:04 PM Dan Bloomberg @.***> wrote:

I don't see the point in most of this.

We have a global L_DISPLAYWITH... that can be set with l_chooseDisplayProg(), but for most situations the default suffices. If you don't want output, use 0 for the dispflag in pixDisplayWithTitle -- we don't need to call l_chooseDisplayProg(L_DISPLAY_WITH_NONE) first. If you want to write the image to file, use pixWrite() -- there's no good way to do it from pixDisplay(), and your code doesn't do it either. There is no place to specify the output filename.

Did you mean to have the default windows display program be "open"? If so, why that and why not

For windows, what is wrong with getting the path on the stack with:

pathname = genPathname(tempname, NULL); _fullpath(fullpath, pathname, sizeof(fullpath));

?

For Windows, we can allow use of the 'open' display program with this variant of your code:

    /* Windows: L_DISPLAY_WITH_IV or L_DISPLAY_WITH_OPEN */
pathname = genPathname(tempname, NULL);
_fullpath(fullpath, pathname, sizeof(fullpath));
if (var_DISPLAY_PROG == L_DISPLAY_WITH_IV) {
    if (title) {
        snprintf(buffer, Bufsize,
                 "i_view32.exe \"%s\" /pos=(%d,%d) /title=\"%s\"",
                 fullpath, x, y, title);
    } else {
        snprintf(buffer, Bufsize, "i_view32.exe \"%s\" /pos=(%d,%d)",
                 fullpath, x, y);
    }
} else {  /* L_DISPLAY_WITH_OPEN */
    snprintf(buffer, Bufsize, "explorer.exe /open \"%s\"", fulllpath);
}

Interesting syntax with the comma in the command line: explorer.exe /open,"%s"

I'll add checks at the top of the function to make sure that the display program is valid for the platform.

— Reply to this email directly, view it on GitHub https://github.com/DanBloomberg/leptonica/pull/683#issuecomment-1476931973, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADCIHUHYJR63W6OREHNZSLW5DA5JANCNFSM6AAAAAAWBOT5RI . You are receiving this because you authored the thread.Message ID: @.***>

DanBloomberg commented 1 year ago

Thank you for the information about how Windows calls display programs.

And you're right, it would be useful to be able to disable pixDisplay() calls!

DanBloomberg commented 1 year ago

commit 0ecd513 Please let me know if is is now working properly for you.

Dan

GerHobbelt commented 1 year ago

👍 merged and working fine.

P.S. I like my STORE mode (so I can run /prog/*, etc. in batch mode without viewers popping up into my face, but still collect the images they were supposed to display), so I merged & mixed it back into my own codebase. But that's just me; won't be all that useful to others.

GerHobbelt commented 1 year ago

Shall I close this as completed, then?