Refactor handling of user input in new/convert commands.

[freakboy3742]

The bulk of prompted user input in Briefcase occurs in the New command, which is inherited by the Convert command. However, there are a handful of other places in Briefcase where the user is asked for an option - and each of these uses has a very slightly different set of semantics around how questions behave.

This PR moves all the user input handling into the Console class, providing a common interface for "asking questions". The New and Convert commands have been refactored to use this common "question asking" interface; as have the uses in macOS (selecting signing identity) iOS and Android (selecting the simulator to use).

As a result of #2114, this interface would also be available for custom bootstraps to use - ensuring that questions asked by custom bootstraps follow the same UX conventions as the rest of Briefcase.

This refactor also provides the opportunity to do some general housekeeping, dramatically simplifying some implementations, and renaming some interfaces so that the public API (or, at least, the API that we encourage people to use) is hopefully a little more clear and consistent, and avoids repetition in naming.

As a result of this change, there may be some cosmetic changes to the order in which options are presented. Previously selection options would be automatically sorted; the new implementation drops that sorting in favor of using provided dictionary order, allowing the user of the API to determine if sorting is appropriate.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[rmartin16]

I agree with this refactor; it is eerily reminiscent of something I attempted early in my journey with BeeWare....but I was too overzealous and unfocused at the time 😅 😆

I do wonder, though, if this should be its final form...

If we consider Console at a time before I started mucking in it so much, its purpose was much clearer: manage basic interaction with the user. In that way, it made much more sense then for why it was attached to the Command with the name input.

At this point, Console has transformed from something that manages user input to something that actually manages the user's console.

So, I wonder if we shouldn't recreate something more akin to what Console was originally; basically, move all the user interaction bits to a new Input class and attach that to the Command as input. Then, Console could become a separate entity on the Command as console, for instance.

This way, invoking the Wait Bar wouldn't require going through command.input (which always felt a bit awkward to me tbh).

Another approach might still create Input but it is attached to Console as input and Console is attached to Command as console. In this way, the Wait bar is available via command.console.wait_bar() and user input is available via command.console.input.text_question().

Additionally, with all these abstractions for soliciting user feedback, should we get rid of input.__call__()? More generally, should we make this primitive method to get input private and instead only use (and advertise) the higher abstractions for use in Briefcase and plugins?

Thoughts?

[freakboy3742]

I agree with this refactor; it is eerily reminiscent of something I attempted early in my journey with BeeWare....but I was too overzealous and unfocused at the time 😅 😆

Great minds, etc? 🤣

I do wonder, though, if this should be its final form...
...
At this point, Console has transformed from something that manages user input to something that actually manages the user's console.

Completely agreed that this corner of the API is a little confusing.

So, I wonder if we shouldn't recreate something more akin to what Console was originally; basically, move all the user interaction bits to a new Input class and attach that to the Command as input. Then, Console could become a separate entity on the Command as console, for instance.

If I understand what you're describing here, then I think it might make sense.

For me, there's essentially four problems to solve here:

1. A naming inconsistency - on Command, self.input = Console(). Is console an input mechanism, or an output mechanism? (or both?)
2. What's the difference between the log, and outputting to console? Why does console.print() not log, but log.info() print to the console?
3. Our "input" generates output. There's one specific manifestation of this problem in this PR - when a validator fails, it really needs to print a WARNING... but Console doesn't know about the logger, so it "fakes" it by printing with a warning-like "bold yellow" style.
4. What even is Printer at this point?

So - what you're proposing is to create a new Input class that becomes the public, documented API for input handling. Purely output methods (e.g., wait_bar and divider) remain on Console; Input gains all the prompt and question asking methods, including the enabled flag.

The constructor for input would take a logger and a console; that way an input handler could raise warnings if needed.

Commands would be constructed with a Console and a Logger; but then build an Input() wrapper around those two.

The Bootstrap API could then be modified to only take an instance of input - the logger and console can be extract from that.

There's 3 downsides I see to this approach.

The first is that there's another class involved - I'm not necessarily convinced another namespace is improving things here.

The second is that the distinction between Logger and Console is a bit vague. Is there a meaningful distinction why you'd say console.print() rather than logger.info()? Would it not make just as much sense to say console.info(), and have that filtered to the underlying log file and/or displayed to the user if necessary?

The third is that the relationship between Console, Logger, and Printer just got more complicated. We've now got four classes. The Printer has a console attribute... that isn't a Console; and in practice, Console and Logger always share a Printer instance. So the question of what Printer is has become even more vague.

This way, invoking the Wait Bar wouldn't require going through command.input (which always felt a bit awkward to me tbh).

Definitely agreed on this - self.input.wait_bar() is an unintuitive API.

Another approach might still create Input but it is attached to Console as input and Console is attached to Command as console. In this way, the Wait bar is available via command.console.wait_bar() and user input is available via command.console.input.text_question().

True - but command.console.input.text_question() is a bit verbose as an API, and I can see that command.input = command.console.input being added as a shortcut, even if only as a migration aid.

Additionally, with all these abstractions for soliciting user feedback, should we get rid of input.__call__()? More generally, should we make this primitive method to get input private and instead only use (and advertise) the higher abstractions for use in Briefcase and plugins?

I'm not 100% sure about this. As I see it, the intention for this method is to retain an analog of the builtin input() method. Although we're not really making any use of that, it makes sense to retain as an API on whatever object is accessible to the user as input() (whether that's command.input() or whatever).

[rmartin16]

There's a lot of organic growth here...

I kinda feel like your points (and past times we've touched on this) are leaning towards just consolidating all of this.

My view of the current state:

• Printer
  • Our most primitive content writing API for the console
  • Transparently manages writing to the console and the log file
• Log
  • Printer wrapper for tracking operations and providing feedback to the user
  • Provides an interface for the log file
• Console
  • Printer wrapper for soliciting user feedback
  • Provides robust console animations

Given this, I could definitely see an argument to combine Log and Console. Log provides these different "levels" of console writing....so, prompting the user could really just become another one of those levels. This would provide unification for Console & Log; in this way, we wouldn't need to track them individually and push them in to plugins and bootstraps independently. In such a model, I'm mostly ambivalent if Printer remains; it could also be subsumed by this new unified class.

With that said, this is one big class... I think part of what was pushing me towards moving the user interaction bits in to a Input class is code mgmt and leaning more on class composition at runtime to combine everything together. After all, in practice, this would just be console.input.text_question() in a bootstrap....or self.input.text_question() if we alias console.input to command.input.

So, do you have a preference in direction in mind? I''m not sure I can tell which way you're leaning...if any way :)

[freakboy3742]

There's a lot of organic growth here...

Oh - completely agreed. Each of the individual steps that we've taken to get here made sense - it's only when steps back and look at where we've arrived (and how what we've got satisfies a new use case) that it makes less sense.

With that said, this is one big class... I think part of what was pushing me towards moving the user interaction bits in to a Input class is code mgmt and leaning more on class composition at runtime to combine everything together.

That's definitely true... but why does that matter? It might make sense to break it up if it made testing or usage easier - if, for example, there was a context where we needed a Logger but not a Console, or if testing a Logger was easier if it wasn't attached to a Console. But in this case, the thing that needs to be mocked is the underlying pieces of Printer (which could equally be the underlying pieces of a combined Printer/Logger/Console class); and while there is some conceptual separation between the APIs offered by Logger and Console, they're all "communicating with the user" APIs, I'm not sure we gain anything by breaking them up - especially if we need to pass 2 or 3 objects around everywhere... and especially if there's internal links between them (so that input classes can write to the log, for example).

Yes, we're going to end up with a single 1000 line class. But that's going to be made up of a dozen or so public API methods, and a dozen or so internal utility methods. That's an entirely testable interface, IMHO; and I'm not sure that some additional language-level classes gives us any organisational clarity that we couldn't get with some inline banner comments separating "sections" of code.

So, do you have a preference in direction in mind? I''m not sure I can tell which way you're leaning...if any way :)

At least for now, my "strong opinion weakly held" position is that merging everything into a Console class, and modifying usage to use command.console.info(), command.console.wait_bar(), and command.console.selection_question() would make sense. It might make sense to preserve a Printer class - but if so, it will be almost entirely as a testing interface. I'll need to dig into the internals a bit to see what falls out.

That said - I'm also open to be convinced otherwise on this.

[mhsmith]

I'm not familiar with this code, so I'll remove myself as a reviewer for now. Feel free to add me back if necessary.

[rmartin16]

With that said, this is one big class...

That's definitely true... but why does that matter?

I think it just makes the code easier to reason about and prevents unnecessary inter-dependence when it's separated in to logical pieces. It also think it makes the code more approachable by newer contributors. I remember trying to take in toga.app.App....it can be a bit daunting as you scroll through hundreds of lines and tens of methods.

my "strong opinion weakly held" position is that merging everything into a Console class [...] would make sense.

I'd support that.

Another implementation strategy could be mix-in classes; right now, each of these things are really only dependent on a Printer instance attached as printer. Or they could all inherit from Printer. In practice, they would always be composed together in to a "super" class. There's nothing really functional about these approaches per se, though....it's more about cognitive overhead and code mgmt. Any of these approaches gets the job done at the end of the day...and I don't think one is inherently better than the rest :) I'm ok with your discretion.

[rmartin16]

Took an initial review and left some comments.

As for potentially extending this refactor, just let me know if you'd like to do that here in this PR....or use this as an incremental step to unblock your other work.

[freakboy3742]

@rmartin16 I've addressed those initial review comments.

In terms of the larger consolidation/refactor - it's going to be a big set of changes; I can't see any way adding those changes to this PR would make it easier to review :-) On that basis, I'd suggest wrapping up the review and feedback process on this PR, and following up quickly with the consolidation PR (I've already started work on it).

[rmartin16]

Minor fixes but lgtm.