Golly bugs

[confocaloid]

You want a nicer error message?

If people believe the feature "File -> Run Clipboard" is useful and should be kept, then I think better error reporting and additional "safety guards" would help.

If the clipboard data doesn't look like valid supported script code, show a message that says so, and refuse to interpret the clipboard in any way.

If the clipboard data does look like valid supported script (Python/Lua), show a warning message saying this:

Code: Select all

The clipboard appears to contain a <insert language name here> script. Do you want to run it?
---
<insert clipboard text here
so that the Golly user can preview it, before deciding
whether or not this should be run>

What if the clipboard contains graphic data instead? (I just tried and got an "Invalid syntax" error message with a very long line consisting of dots.
I would not want to get tricked into running some code just by trying to "Run Clipboard" a picture.)

Even if the script is valid, a Golly user might accidentally copy the wrong script from a page, or copy an unrelated script from the wrong page while switching between different applications. I think previewing the clipboard and better error reporting would help.

[dvgrn]

If the clipboard data does look like valid supported script (Python/Lua), show a warning message saying this:

Code: Select all

The clipboard appears to contain a <insert language name here> script. Do you want to run it?
---
<insert clipboard text here
so that the Golly user can preview it, before deciding
whether or not this should be run>

Just for the record, in my day-to-day Golly workflow I use File > Run Clipboard very regularly, especially when I'm test-running code snippets. I've even mapped the backtick key in the upper left corner of the keyboard to File > Run Clipboard, to make it easier to access the command.

If Golly ever added a "preview" warning message like the above, I would definitely want a Preferences checkbox where it could be turned off. The warning message would never be anything but an annoyance to me. I only use File > Run Clipboard just after I've copied a script to the clipboard, so I've already seen the script in question, just a second or two before. I want Golly to run the script, not show it to me again.

The possible failure modes involving copying the wrong script accidentally, or (especially) executing File > Run Clipboard on an image, don't seem plausible enough to spend any time accounting for them. A non-trivial piece of new preview-pane functionality would have to be added. There aren't any known image files that will run successfully as a script if File > Run Clipboard is executed on them. It's not clear that anyone has ever accidentally run File > Run Clipboard on an image file, anyway, so the current error message seems fine for that highly improbable situation.

Now, the misidentification of RLE as a Perl script _does_ happen quite a lot, I suspect -- it certainly happens to me a lot. So it makes sense to me to remove the reference to Perl scripts, and optionally add a check for a valid RLE header instead, and give a friendly explanation of what is probably going wrong in that case.

[confocaloid]

Just for the record, in my day-to-day Golly workflow I use File > Run Clipboard very regularly, especially when I'm test-running code snippets. I've even mapped the backtick key in the upper left corner of the keyboard to File > Run Clipboard, to make it easier to access the command. [...]

Well I can see some good potential for errors, when the keys "Backtick" and "Tab" are adjacent on the keyboard. Tab is mapped to "Next Step" and the user might want to "run for one step, then run script", but instead accidentally run the script immediately. What happens in that case, and whether that error can be undone, depends on what exactly the script does.

That said, it's clear that different people have different workflows. Sometimes a warning/preview message is dubbed "annoyance". Other times, a warning/preview would be very very helpful, to avoid frequent errors and to allow the Golly user to make sure they are doing the right thing.

[confocaloid]

deleted, likely needs more work

[Andrew]

I think previewing the clipboard and better error reporting would help.

Surely the people who want to preview the clipboard can just open it in their text editor (Mac users can also use the Finder to Edit > Show Clipboard). Requiring confirmation would be incredibly annoying for people like me who regularly run small pieces of Lua/Python code.

But we do need to improve the script detection code to handle the case where the clipboard contains a pattern, and definitely remove the reference to Perl. (Actually, it's probably time to remove the optional Perl support from Golly. I seriously doubt anyone is using Perl with Golly these days, so I'll check with the other devs to see if anybody objects to me doing that.)

@Dave/Chris: The code in MainFrame::GetScriptFileName needs a major overhaul so best leave that job to me. My thinking is to just simplify the code in the following way: If a line starts with "--" or "local " then assume it's a Lua script, or if a line starts with "import " or "from " then assume it's a Python script. If the clipboard doesn't meet any of those conditions then assume it's a pattern and display a Note saying something like "The clipboard seems to contain a pattern rather than a Lua/Python script, so try using Open Clipboard rather than Run Clipboard.". This will handle clipboards that contain macrocell/MCell patterns, not just RLE. Thoughts?

[rowett]

@Dave/Chris: The code in MainFrame::GetScriptFileName needs a major overhaul so best leave that job to me. My thinking is to just simplify the code in the following way: If a line starts with "--" or "local " then assume it's a Lua script, or if a line starts with "import " or "from " then assume it's a Python script. If the clipboard doesn't meet any of those conditions then assume it's a pattern and display a Note saying something like "The clipboard seems to contain a pattern rather than a Lua/Python script, so try using Open Clipboard rather than Run Clipboard.". This will handle clipboards that contain macrocell/MCell patterns, not just RLE. Thoughts?

Sounds good. Perhaps: "The clipboard does not contain a Lua or Python script. If it contains a pattern you want to open try using Open Clipboard instead."

[Andrew]

Sounds good. Perhaps: "The clipboard does not contain a Lua or Python script. If it contains a pattern you want to open try using Open Clipboard instead."

Yep, that would be better. If the clipboard doesn't contain a script then we can't assume it contains a pattern. Might be some random text.

[confocaloid]

safeopenclip.zip

corrected several typos in messages

(3.9 KiB) Downloaded 5 times

runclipboard.png (19.5 KiB) Viewed 385 times

Actually, it's probably time to remove the optional Perl support from Golly.

Compiled and run successfully under Ubuntu 22.04. utils.h, utils.cpp still refer to Perl, is that intended?

[Andrew]

utils.h, utils.cpp still refer to Perl, is that intended?

No -- now removed. Also fixed the typos in safeopenclip.lua. Thanks!

[confocaloid]

Currently Lua scripts can use the scripting command "g.savechanges(query, message)" to show a sort of a confirmation dialog, where there are three buttons (yes, no, cancel). Neither of the buttons aborts the script immediately; instead, clicking on one of buttons returns the corresponding value.

AFAIK this is not available for Python scripts. (Is this intended?) In addition, the name of the Lua scripting command suggests that its use was intended to be limited to asking the user whether they want to save some changes.

My suggestion is to add a general-purpose scripting command for confirmation dialogs for Python/Lua scripting. Here are two possible syntaxes:

Code: Select all

g.confirm(query, message, showCancel = true)
    Show a confirmation dialog and return "yes", "no" or "cancel" depending on which button the user clicked.
    If showCancel = false, then only "Yes" and "No" buttons are shown.

or

Code: Select all

g.confirm(query, message, textYes = "Yes", textNo = "No", textCancel = "Cancel")
    Show a confirmation dialog and return "yes", "no" or "cancel" depending on which button the user clicked.
    Captions of buttons can be changed by setting textYes, textNo, textCancel.
    If textCancel is set to the empty string, then only two buttons are shown.

I think confirmation dialogs are useful in enough situations, not limited to "Do you want to save changes?"-type dialogs. For example, before doing some operation, the script can let the user choose whether the selection should be randomized. "Yes" would randomize the selection and proceed; "No" would proceed without changing the selection; "Cancel" would cancel the operation (but not necessarily abort the script immediately -- there may be another operation, or some kind of cleanup).

savechanges.png (27.83 KiB) Viewed 336 times

[Andrew]

Currently Lua scripts can use the scripting command "g.savechanges(query, message)" ...
AFAIK this is not available for Python scripts. (Is this intended?)

More or less. It was needed in a couple of Lua scripts (along with g.settitle) that use the overlay (also not available in Python). I don't have much interest in Python any more.

My suggestion is to add a general-purpose scripting command for confirmation dialogs for Python/Lua scripting.

I quite like this idea, and actually came close to implementing something like it a few years ago. I prefer your 2nd suggestion, but I'd add another parameter that lets you specify which of the 3 buttons is the default (ie. selected by the return/enter key):

Code: Select all

g.confirm(query, message, textYes = "Yes", textNo = "No", textCancel = "Cancel", default = "Yes")

I might be able to summon up the enthusiasm to squeeze this into 4.3b2.

[Andrew]

I might be able to summon up the enthusiasm to squeeze this into 4.3b2.

I've added a tentative Lua version of this idea. I decided to call the function "query" rather than "confirm". Also dropped the idea of an extra parameter to set the default button, mainly because you can't make the Cancel button the default on macOS, and if you want to make No the default button you simply need to swap the labels for Yes and No.

Here's some code I use to test it. Works fine on macOS but haven't yet tried it on Windows/Linux:

Code: Select all

local g = golly()
g.show( g.query("Do something?", "Click on a button or hit the escape key to Cancel.") )
g.show( g.query("Try again?", "Only Yes or No.", "Yes", "No", "") )
g.show( g.query("Try yet again?", "Only Yes or No but No is the default.", "No", "Yes", "") )
g.show( g.query("Try one more time?", "Buttons have different labels.", "Do It!", "Negative", "Abort") )
g.show( g.query("Save changes?", "Your changes will be lost if you don't save!", "Save", "Don't Save") )

Note that the function simply returns the label of the selected button.

One slight annoyance I noticed (possibly only on macOS?): If you change the label of the Cancel button then the escape key beeps rather than closing the dialog.

[rowett]

Here's some code I use to test it. Works fine on macOS but haven't yet tried it on Windows/Linux

If I create a two button dialog without a Cancel button and press Escape then on Windows the keypress is ignored but on Linux the dialog closes and the string gtk-cancel is returned.

[Andrew]

If I create a two button dialog without a Cancel button and press Escape then on Windows the keypress is ignored but on Linux the dialog closes and the string gtk-cancel is returned.

Yep, I see the same thing on my Linux system -- how annoying! I don't see any way around that problem. Might have to always have a Cancel button on Linux and just mention that in the docs. Any other options?

[rowett]

If I create a two button dialog without a Cancel button and press Escape then on Windows the keypress is ignored but on Linux the dialog closes and the string gtk-cancel is returned.

Yep, I see the same thing on my Linux system -- how annoying! I don't see any way around that problem. Might have to always have a Cancel button on Linux and just mention that in the docs. Any other options?

Another option would be to detect the gtk-cancel in g_query() and relaunch the dialog. Not sure it's better than having to have a Cancel button.

Code: Select all

static int g_query(lua_State* L)
{
    AUTORELEASE_POOL
    CheckEvents(L);

    bool finished = false;
    wxString query = wxString(luaL_checkstring(L, 1), LUA_ENC);
    wxString msg = wxString(luaL_checkstring(L, 2), LUA_ENC);

    wxString labelYes =    _("Yes");
    wxString labelNo =     _("No");
    wxString labelCancel = _("Cancel");
    if (lua_gettop(L) > 2) labelYes     = wxString(luaL_checkstring(L, 3), LUA_ENC);
    if (lua_gettop(L) > 3) labelNo      = wxString(luaL_checkstring(L, 4), LUA_ENC);
    if (lua_gettop(L) > 4) labelCancel  = wxString(luaL_checkstring(L, 5), LUA_ENC);

    long style = wxICON_INFORMATION | wxCENTER | wxYES_NO;
    if (labelCancel.length() > 0) style = style | wxCANCEL; // add Cancel button

    wxMessageDialog dialog(wxGetActiveWindow(), msg, query, style);

    if (labelCancel.length() > 0) {
        dialog.SetYesNoCancelLabels(labelYes, labelNo, labelCancel);
    } else {
        dialog.SetYesNoLabels(labelYes, labelNo);
    }

    while (!finished) {
        finished = true;

        int button = dialog.ShowModal();
        if (viewptr) viewptr->ResetMouseDown();
        switch (button) {
            case wxID_YES: {
                wxString label = dialog.GetYesLabel();
                lua_pushstring(L, (const char*)label.mb_str(LUA_ENC));
                break;
            }
            case wxID_NO: {
                wxString label = dialog.GetNoLabel();
                lua_pushstring(L, (const char*)label.mb_str(LUA_ENC));
                break;
            }
            case wxID_CANCEL: {
                wxString label = dialog.GetCancelLabel();
                if (label == wxString("gtk-cancel")) {
                    finished = false;
                } else {
                    lua_pushstring(L, (const char*)label.mb_str(LUA_ENC));
                }
                break;
            }
            default: {
                // should never happen
                GollyError(L, "query bug: unexpected button.");
            }
        }
    }

    return 1;   // result is a string
}

[Andrew]

Another option would be to detect the gtk-cancel in g_query() and relaunch the dialog.

Hmm, that's rather ugly. I'm going to try binding a key event handler to the dialog so we can detect the escape key and ignore it when there's no Cancel button. Late here so will try that tomorrow.

[confocaloid]

Here's some code I use to test it. Works fine on macOS but haven't yet tried it on Windows/Linux:

Code: Select all

local g = golly()
g.show( g.query("Do something?", "Click on a button or hit the escape key to Cancel.") )
g.show( g.query("Try again?", "Only Yes or No.", "Yes", "No", "") )
g.show( g.query("Try yet again?", "Only Yes or No but No is the default.", "No", "Yes", "") )
g.show( g.query("Try one more time?", "Buttons have different labels.", "Do It!", "Negative", "Abort") )
g.show( g.query("Save changes?", "Your changes will be lost if you don't save!", "Save", "Don't Save") )

Note that the function simply returns the label of the selected button.

Ubuntu 22.04:

• Going through all five messages by always pressing <Esc> produces sequence "Cancel", "gtk-cancel", "gtk-cancel", "Abort", "Cancel".
  Using <Alt+F4> or the close window button has the same effect.
• Always pressing <Enter> produces sequence "Yes", "Yes", "No", "Do It!", "Save".
• Also tested with the following message:

  Code: Select all

  local g = golly()
  local msg     = "Put a random two-state soup in the selected region?"
  local textYes = "Randomize selection"
  local textNo  = "Leave selection unchanged"
  
  g.show(g.query("Example", msg, textYes, textNo, ""))
  -- <Esc> or close window button returns "gtk-cancel"
  -- clicking a button returns its label (textYes or textNo)
  
  g.show(g.query("Example", msg, textYes, textNo, "Undo all changes"))
  -- <Esc> or close window button returns "Undo all changes",
  -- clicking a button returns its label (textYes or textNo or "Undo all changes")

edit: maybe the function g_query should return the value of corresponding labelYes/labelNo/labelCancel local variable, instead of doing dialog.GetYesLabel() to extract the value back from the user interface?

Code: Select all

// preceding code unchanged
    switch (button) {
        case wxID_YES: {
            wxString label = labelYes;
            lua_pushstring(L, (const char*)label.mb_str(LUA_ENC));
            break;
        }
        case wxID_NO: {
            wxString label = labelNo;
            lua_pushstring(L, (const char*)label.mb_str(LUA_ENC));
            break;
        }
        case wxID_CANCEL: {
            wxString label = labelCancel;
            lua_pushstring(L, (const char*)label.mb_str(LUA_ENC));
            break;
        }
        default: {
            // should never happen
            GollyError(L, "query bug: unexpected button.");
        }
    }
// following code unchanged

When there is no Cancel button, <Esc> or closing the dialog would return the empty string instead of "gtk-cancel".

[Andrew]

When there is no Cancel button, <Esc> or closing the dialog would return the empty string instead of "gtk-cancel".

But that's not consistent with what happens on the other platforms. If a script writer creates a 2-button dialog then we want g.query to return the label on one of those buttons (usually Yes/No). I don't want people to have to use g.os() to check for a different response if the script is running on Linux.

I couldn't get my key event handler to work -- it never gets called (probably because the escape key detection is done inside a low-level gtk routine rather than in wx code), so I think we'll have to go with Chris's solution of displaying the dialog again until the user selects one of the 2 buttons. Mildly annoying for Linux users but probably won't happen all that often, especially because we already have g.note and g.warn for the cases where Cancel and OK buttons are sufficient.

[rowett]

Another platform difference I've noticed is that Linux translates "gtk-yes", "gtk-no" and "gtk-cancel" to button captions "Yes", "No" and "Cancel" respectively. Fortunately it still returns "gtk-yes" if "Yes" is clicked on.

Windows does not translate so if you specify "gtk-yes" you get that on the button.

Code: Select all

local g = golly()
g.show( g.query("Try yet again?", "What are the button labels?", "gtk-yes", "gtk-no", "gtk-cancel") )

[tommyaweosme]

backspace doesnt work right on the web version