askMulti multiple question dialog support #4671

skef · 2021-03-19T12:24:20Z

From the annals of Probably Not Worth It In Retrospect ...

Given the added Techref I think this is pretty self-explanatory. I recommend pulling this over, building the documentation, and pointing your browser to the local files.

I appear to have a bit more window-sizing work to do, as the window described in the techref doesn't quite come up the right size at present, but otherwise things seem to be in good shape. I've tested with both the GDK and X11 back-ends, although more modestly with the latter.

If you don't want to fool around building your own lists/dicts I've made a package with some convenience routines: https://github.com/skef/fontforge_helper . But you can also just test with
an init script something like this:

def raiseMulti(u, font):
    try:
        ans = fontforge.askMulti("blah", [
            { 'type':'choice', 'question':'Why?', 'checks':True, 'answers':[ { 'name':'_one', 'default': True }, { 'name':'_two' }, { 'name':'thr_ee' }, { 'name': 'four'}, { 'name': 'five' }, { 'name': 'six' }] },
            { 'type':'choice', 'question':'How?', 'checks':True, 'multiple': True, 'answers':[ { 'name':'a', 'default': True }, { 'name':'_b' }, { 'name':'c', 'default': True }] },
            { 'type':'openpath', 'question':'What _file?', 'tag':'blah', 'default':'/tmp/tt.txt', 'filter':'*.txt' },
            { 'type':'savepath', 'question':'What\n_save\nfile?', 'tag':'foo', 'noalign': True, 'default':'bbb' },
            { 'type':'choice', 'tag':'Who?', 'multiple':True, 'answers':[ { 'name':'me' }, { 'name':'you' }, { 'name':'them' }, { 'name': 'blah' }] },
            { 'type':'string', 'question':'_Which one?', 'tag':'bar', 'default':'because' },
            ] )
    except Exception as inst:
        print(inst)
    print(ans)

def raiseMultiCat(u, font):
    cats = [ { 'category': '_First  ', 'questions':
        [ { 'type':'choice', 'question':'Why?', 'checks':True, 'answers':[ { 'name':'one', 'default': True }, { 'name':'two' }, { 'name':'three' }, { 'name':'four' }, { 'name':'five' }, { 'name':'six' }, { 'name':'seven' }, { 'name':'eight' }, { 'name':'nine' }] },
          { 'type':'choice', 'question':'How?', 'checks':True, 'multiple': True, 'answers':[ { 'name':'a', 'default': True }, { 'name':'b' }, { 'name':'c', 'default': True }] },
        ]
      },
      { 'category': '_Second  ', 'questions':
        [ { 'type':'openpath', 'question':'What file?', 'tag':'blah', 'default':'/tmp/tt.txt', 'filter':'*.txt' },
          { 'type':'savepath', 'question':'What save file?', 'tag':'foo', 'default':'bbb' },
          { 'type':'choice', 'question':'Who?', 'answers':[ { 'name':'me' }, { 'name':'you' }] },
          { 'type':'string', 'question':'Which one?', 'tag':'bar', 'default':'because' },
        ]
      },
     ]
    try:
        ans = fontforge.askMulti("two blah", cats)
    except Exception as inst:
        print(inst)
    print(ans)

fontforge.registerMenuItem(raiseMulti, None, None, ("Font"), None, "Raise Multi Dialog")
fontforge.registerMenuItem(raiseMultiCat, None, None, ("Font"), None, "Raise Multi Cat")

Mnemonic support is partial. If you add a mnemonic to a checkbox or radio button it works (although there is no visual change, you can press space to select/toggle selection). If you add one to a string or path question the field should get the focus. Mnemonics don't currently work with GTabSets, see #4670.

You can also focus a choice-list and use the arrows to change the selection on a non-multiple choice list. There doesn't seem to be a sane way of managing selection by keyboard for a multi-choice-list.

I try to size the initial window reasonably.

The layout choices are mostly the result of trying different options. I wanted to have the dimensions be quite variable so its easier to look at it and the Font or Char View at the same time when desired. Hence GFlowBox and GScroll1Box. The advantage of having the text fields scale to the end of the window is that it's an easy work-around for longer strings -- the user can just tug at the window if they need to.

skef · 2021-03-19T12:30:54Z

I think the NoUI build failure is spurious/unrelated -- I tried it on my box and it works OK.

lgtm-com · 2021-03-19T12:55:24Z

This pull request introduces 6 alerts when merging ef3d493 into 8adce5f - view on LGTM.com

new alerts:

5 for Ambiguously signed bit-field member
1 for Declaration hides parameter

jtanx · 2021-03-19T21:35:07Z

The CI build is fixed in #4669, it comes from github bumping the image to Ubuntu 20.04 from 18.04

skef · 2021-03-19T22:27:29Z

@jtanx That didn't quite seem to do it. NoUI build still failing for seemingly PR-unrelated reasons.

jtanx · 2021-03-19T22:31:47Z

It's still the same thing, you can see the gettext error:

-- Configuring incomplete, errors occurred!
  Could NOT find Gettext (missing: GETTEXT_MSGMERGE_EXECUTABLE
See also "/home/runner/work/fontforge/fontforge/repo/build/CMakeFiles/CMakeOutput.log".
  GETTEXT_MSGFMT_EXECUTABLE)
Call Stack (most recent call first):
  /usr/local/share/cmake-3.19/Modules/FindPackageHandleStandardArgs.cmake:582 (_FPHSA_FAILURE_MESSAGE)
  /usr/local/share/cmake-3.19/Modules/FindGettext.cmake:81 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:100 (find_package)

Seems like GHA doesn't update to the latest HEAD when you rerun:

HEAD is now at 8f7735a Merge ef3d4934d34fafa0c1fe5badf58e971ea0e3a20c into 8adce5f70ee8b082b8c39fe174ea3add4d00d342

8adce5f is the commit before my merge.

Try close and reopen the PR? Or rebase to master.

skef · 2021-03-20T03:08:22Z

Rebasing seems to have worked, thanks!

lgtm-com · 2021-03-20T03:30:23Z

This pull request introduces 6 alerts when merging c0da058 into 25c3b74 - view on LGTM.com

new alerts:

5 for Ambiguously signed bit-field member
1 for Declaration hides parameter

skef · 2021-03-20T10:30:56Z

BTW @jtanx I eventually landed on moving the children of a GScroll1Box within the nested window because I didn't come up with a better solution, possibly out of ignorance. It seems like one should be able to pick the displayed region of a window, but maybe not.

If there's a more standard or just better way of doing that let me know.

jtanx

I'm on the fence for this one. This is getting to the point of being more a way to generate a complex modal / define a UI with gdraw rather than a pure 'ask' dialog you'd normally associate with what standard UI toolkits provide.

If you want to extend it, then I guess I won't stop you, but definitely for myself, I won't be doing any work towards extending gdraw, if anything I'd be trying to cut it back, like in #4664.

I do worry though, how much more difficult this will make future porting efforts, but I guess that too is a very long term goal, if at all possible.

doc/sphinx/techref/askmulti.rst

skef · 2021-03-21T00:03:15Z

I'm on the fence for this one. This is getting to the point of being more a way to generate a complex modal / define a UI with gdraw rather than a pure 'ask' dialog you'd normally associate with what standard UI toolkits provide.

Yes, that's specifically the point. It's intended to handle 80% of requests like #3663. (This has been requested by others too -- I just can't turn up the links at the moment.)

I do worry though, how much more difficult this will make future porting efforts, but I guess that too is a very long term goal, if at all possible.

So GDK is a dead-end, GTK-4 is not currently being pursued because its too different (personal communication), but we might port to Qt someday (because that wouldn't be too different?) and therefore enhancement of UI functionality should stop until then? Do we have a year for that? A decade?

Your UI porting work is more than admirable, it's beyond the call, really. But the whole project shouldn't just revolve around it. We desperately need to shift a large hunk of the extension burden off of C and the main dev folks here onto Python and other users. This feature is needed to do that.

jtanx · 2021-03-21T00:24:39Z

Like yeah, for sure that's why I'm saying I won't get in the way of gdraw changes if someone wants to do them. But at the same time I just want to make sure that (a.) there is a genuine use case (which you've highlighted), and (b.) we aren't digging the hole deeper for ourselves unnecessarily.

For GFlowBox/GScroll1Box, can you go into further detail about what they solve/what you can't do with the existing controls?

skef · 2021-03-21T00:24:41Z

Sorry, I realize you mostly made the same point I'm making.

There are some users who may be happy to design and raise their own tkinter windows but that's a pretty high bar developer-wise. I don't feel like we should be recommending that as a solution. I put a fair amount of thought and (obviously, I think) a hefty dollop of work into this approach and I think it's the better way forward.

I also suspect you underestimate how "custom" and strange the rest of the UI is and therefore how much work it would/will be to swap everything to a new widget set. In my view whatever additional work this involves will be dwarfed by the general problem.

skef · 2021-03-21T00:46:15Z

For GFlowBox/GScroll1Box, can you go into further detail about what they solve/what you can't do with the existing controls?

There's one part of this feature yet to implement that there are little pieces of, which is an optional apply callback and "protocol" that lets you make a dialog work the way the Transform dialog does. (That's what doUndoLayer() will be for.) Partly for that reason, and partly for general layout reasons, I wanted these dialogs to be usefully resizable, so that you can see them and other windows at the same time, potentially not on a large screen.

(One of the general layout reasons has to do with localization: the window designer doesn't necessarily know how long the individual strings will be so I wanted to remove the need to think about that as much as possible.)

That's more or less what GFlowBox is for. Most widget sets have something like it. You narrow the container and the contents reflow, potentially with justification. If there's extra room the contents may spread out. (The relevant contents in this case will be radio buttons and checkboxes -- those are the only things that can spread out. I also provide the list-like widget used in askChoices() as an alternative but I find it to be kind of crappy.)

Before these new boxes the only real general layout box is GHVBox, and it a) insists that the desired size of its contents should be the minimum size and b) doesn't have integrated scrolling. That means you can resize bigger but not really smaller -- the window just truncates. So I needed a container to put GFlowBoxes into in that could be genuinely resized and would scroll when needed.

Frankly I was shocked that I had to write GScroll1Box: I thought there would already be some kind of scrolling container in gdraw. But it turned out that while there was (clearly) already a scrollbar widget there was no generic scrolling container: every use of a scroll bar is specific. The GList fakes its container aspect with how it paints. So does GMatrixEdit, although it works quite differently.

Anyway, that's why there's GScroll1Box. It's filling an implementation hole.

skef · 2021-03-21T01:00:15Z

FWIW I don't think we should invest much time in improving gdraw either, and when we do it should be in the direction of what other widget sets are likely to already have (within reason). But there are two things I really wish it had.

More consistent support of mnemonics, along the lines of GTabSet doesn't support mnemonics or other key acceleration #4670.
More visual feedback about focus, again for mnemonics

Issue 2 raises a really silly situation: There's all this custom code to draw boxes of infinite variety around widgets, as evidenced in the X Resource Editor, and there's already a handle_focus GWidget method to receive a focus event, but the stupid radio buttons and checkboxes don't visually change when they're focused! What on Earth?

frank-trampe · 2021-04-14T17:52:51Z

fontforge/python.c

+	    k = qspec->tag;
+	    assert( !PyDict_Contains(r, k) );
+	    // XXX seems like v should have an extra reference at this point,
+	    // (because _SetItem() doesn't steal) but DECREFing v causes crashes


I don't entirely understand what this means, but are you sure you don't need to increment the reference count on Py_None?

That had been my impression but I guess that impression was wrong. I'll add the Py_None reference counting.

PyTuple_SetItem() is documented as "stealing" a reference, hence the reference increment before the call. PyDict_SetItem() is documented as not stealing a reference: https://docs.python.org/3/c-api/dict.html#c.PyDict_SetItem , which in my mind means it should take a reference, which means that I should be decrementing my own reference to it after the call, but that causes crashes. So there's something I'm missing.

Ref count should be fixed

frank-trampe · 2021-04-14T18:25:47Z

fontforgeexe/multidialog.c

+    } else {
+        return false;
+    }
+    return true;


I don't understand. Why can't we just drop the else and have a return false line?

I'm just copying a common semantic from other handlers.

Looking through the code it I can't find an easy example of an event handler return value being attended to, so maybe it's irrelevant? I have been assuming that true "consumes" the event and false means the processing might continue to higher or lower handlers.

Can @jtanx offer any clarification?

It's used e.g. here

fontforge/gdraw/gcontainer.c

Line 356 in e873b8f

handled = (gd->e_h)(gw,event);

to determine whether or not an event was handled, so yes your interpretation is right, I think. The gcontainer stuff was always a bit complicated, so not entirely sure how much of it actually ends up being used/important though

agree with dropping the redundant else though.

Oh, I'm guessing this was my reminder to do something about the F1/Help link, but I didn't decide whether to link to the multidialog help page (potentially useful but counter-intuitive) or allow the caller to supply their own help somehow.

I'll fix it one way or another.

frank-trampe · 2021-04-14T18:26:55Z

fontforgeexe/multidialog.c

+
+enum multi_done_state { mds_not = 0, mds_ok = 1, mds_cancel = 2, mds_apply = 3 };
+
+static int Multi_Button(GGadget *g, GEvent *e) {


A comment explaining the wiring scheme might be helpful.

I added some relevant comments

frank-trampe · 2021-04-14T19:43:16Z

fontforgeexe/multidialog.c

+    return gcd;
+}
+
+static void MultiSyncChoices(MultiDlgSpec *spec) {


Is this name descriptive? It seems to me that the function just sets defaults.

I'll change it to MultiSetDefaults() then.

Changed the function name

frank-trampe · 2021-04-14T19:48:58Z

fontforgeexe/multidialog.c

+        qstn = (MultiDlgQuestion *) GGadgetGetUserData(g);
+        free(qstn->str_answer);
+        qstn->str_answer = GGadgetGetTitle8(g);
+        if (*qstn->str_answer == '\0') {


What necessitates this behavior?

Code that deals with GGadgetGetTitle8() in other places tends to do this. I assume it's because if the content of the field goes from non-empty to empty the GDraw routines don't null out the string for you. One could always fix the problem there, of course.

frank-trampe · 2021-04-14T19:49:32Z

fontforgeexe/multidialog.c

+    }
+}
+
+static void MultiCopyStrings(GWindow gw, int strcid, int fbpair) {


And this function looks like it "syncs".

Also changed this function name

frank-trampe · 2021-04-14T20:21:11Z

@skef, could you write up the theory of operation for the flow dialog? It would make it a lot easier for me to review that portion of the code.

skef · 2021-04-14T22:22:52Z

@skef, could you write up the theory of operation for the flow dialog? It would make it a lot easier for me to review that portion of the code.

I'm not sure what you mean by "theory of operation". At a high level the idea in SAT-analogy syntax is just " GHVBox : GtkGrid :: GFlowBox : GtkFlowBox", although I didn't look at the code of the latter, or really the API, to implement GFlowBox.

Beyond that the main point of complication is the integrated "label".

If you ignore that label the goal is just to flow the "contents" (GGadgets) like text is flowed, so perhaps left-, right-, or center-aligned or spaced out. And in the opposite direction if there is extra space you might want the content to sit at the top, center, bottom, or be similarly spaced out. The difference is that in the primary direction (horizontal in all existing uses) you're "wrapping" and in the secondary or "opposite" direction you're winding up with however many "lines" result from that wrapping.

The code is arranged in terms of "normal" and "oppo" because you might want the primary alignment (the "wrapping") to be vertical or horizontal, but the unused vertical code is currently incomplete. That's basically because I couldn't decide what to do about the label in some of those cases. (It wouldn't be hard to get a label-less vertical flowbox working -- it's already very close.)

In horizontal mode there's an optional label that is hard-coded to appear on the left. You can also adjust the label alignment within its "area".

Beyond that the main "operational" complication is how it reports its "desired" size. GHVBoxes tend to treat the desired size as the minimum size, so the GFlowBox accordingly tends to report its desired size as the minimum size in its wrapping direction (in this case horizontal) and a single "line" in the opposite direction (because GHVBoxes are generally happy to be larger). The former is typically going to be the width of the largest element. There are some flags in the underscore version of the Desired call to get different desired sizes depending on what you want to know.

There's all sorts of things I could go into, like some of the scrollbar calculation weirdness, but that's the basic idea.

If I have to write a full detailed "manual" or equivalent code commends to get this integrated I will, but obviously that would take a fair bit of effort and be more documentation than almost any other code in the project.

skef · 2021-04-15T00:33:13Z

@frank-trampe If what you're asking for is more of a quick get-one's-footing primer on how GDraw containers interact with what they contain and what contains them I guess I could write something up.

frank-trampe · 2021-10-05T14:17:58Z

Other than the conflict from #4809, I think that this is good to go.

frank-trampe · 2021-10-05T14:21:19Z

@jtanx, would you look at the conflict? It seems to me that the right thing is just to remove any case in which text_in_resource is truthy, but I want to be sure I'm not missing anything.

LGTM fixes

jtanx · 2021-10-09T08:19:55Z

fontforge/python.c

+    MultiDlgSpec dlg;
+
+    if ( no_windowing_ui ) {
+	PyErr_Format(PyExc_EnvironmentError, "No user interface");


This leaks the above objects

jtanx · 2021-10-09T08:22:11Z

Rebased to fix that conflict, I still hold my reservations about this, but let's merge.

frank-trampe · 2022-01-14T20:02:56Z

Any reason we can't merge this?

ctrlcctrlv · 2022-01-16T07:19:51Z

No. I tried to give be498f6 a more helpful message than just a commit list via the squash mechanism:

[Python] Add askMulti (multiple question dialog) (#4671)

fontforge.askMulti can now be passed a list() of dict()'s, which can also be categorized, to create dialog boxes that ask multiple questions at once.

Author: @skef
Reviewer: @jtanx

This is a profoundly useful feature despite reservations and I'm glad @frank-trampe asked the question.

@skef

`fontforge.askMulti` can now be passed a list() of dict()'s, which can also be categorized, to create dialog boxes that ask multiple questions at once. Author: @skef Reviewer: @jtanx

skef force-pushed the sqdialog branch from ef3d493 to c0da058 Compare March 20, 2021 02:56

jtanx reviewed Mar 20, 2021

View reviewed changes

doc/sphinx/techref/askmulti.rst Outdated Show resolved Hide resolved

skef mentioned this pull request Mar 21, 2021

Discoverable Python plugin support #4642

Merged

frank-trampe reviewed Apr 14, 2021

View reviewed changes

skef force-pushed the sqdialog branch from f16e16b to 2a56414 Compare April 15, 2021 00:05

skef mentioned this pull request Jul 1, 2021

community for fontforge #4762

Open

askMulti multiple question dialog support

7f451e6

skef added 4 commits October 9, 2021 16:02

Fix initial window size problem

ffe03ed

LGTM fixes

Run rebuild docs-manifest instead of hand-editing

1547df7

Remove extra backquotes

b6b5ea4

Code review responses

832db66

jtanx force-pushed the sqdialog branch from 2a56414 to 832db66 Compare October 9, 2021 08:18

jtanx reviewed Oct 9, 2021

View reviewed changes

ctrlcctrlv merged commit be498f6 into fontforge:master Jan 16, 2022

ctrlcctrlv mentioned this pull request Jan 27, 2022

2021–2022 release #4840

Closed


		enum multi_done_state { mds_not = 0, mds_ok = 1, mds_cancel = 2, mds_apply = 3 };

		static int Multi_Button(GGadget g, GEvent e) {

askMulti multiple question dialog support #4671

askMulti multiple question dialog support #4671

Uh oh!

Conversation

skef commented Mar 19, 2021

Uh oh!

skef commented Mar 19, 2021

Uh oh!

lgtm-com bot commented Mar 19, 2021

Uh oh!

jtanx commented Mar 19, 2021

Uh oh!

skef commented Mar 19, 2021

Uh oh!

jtanx commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skef commented Mar 20, 2021

Uh oh!

lgtm-com bot commented Mar 20, 2021

Uh oh!

skef commented Mar 20, 2021

Uh oh!

jtanx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skef commented Mar 21, 2021

Uh oh!

jtanx commented Mar 21, 2021

Uh oh!

skef commented Mar 21, 2021

Uh oh!

skef commented Mar 21, 2021

Uh oh!

skef commented Mar 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frank-trampe commented Apr 14, 2021

Uh oh!

skef commented Apr 14, 2021

Uh oh!

skef commented Apr 15, 2021

Uh oh!

frank-trampe commented Oct 5, 2021

jtanx commented Mar 19, 2021 •

edited

Loading