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

Enable adding custom input lines #1897

Closed
Kebap opened this issue Aug 17, 2018 · 17 comments · Fixed by #4055
Closed

Enable adding custom input lines #1897

Kebap opened this issue Aug 17, 2018 · 17 comments · Fixed by #4055

Comments

@Kebap
Copy link
Contributor

Kebap commented Aug 17, 2018

Brief summary of issue / Description of requested feature:

When creating a miniconsole/userwindow, it would be nice for it to have a seperate input line of its own, where you can input text, which will then be processed by functions for that specific window.

Steps to reproduce the issue / Reasons for adding feature:

  1. Create userwindow
  2. Show content & try to interact
  3. You need to use the central input line of your profile, which will send text to game and other alias if you do not prepare

Error output / Expected result of feature

  1. Miniconsole can have a custom input line. All text entered there will not go to game or alias, instead be available for minicolsole functionality.

Extra information, such as Mudlet version, operating system and ideas for how to solve / implement:

Discussion on Discord with demonnic, Kasa, Vadi, etc.

demonnic: I've always wanted to have a secondary input box that maybe raised a specific event. so you could chat in a separate window, or input below your chat and have it process based on which tab is active, etc. Would make YATCO potentially a dockable widget, which would be nice, especially if we could get that input option.

Kasa: Do you mean input as in, like, a separate text bar that executes some command taking the text as an argument? I mean floating

demonnic: yes, or raises an event you specify in the creation with the text as an argument, either or

Kasa: Right, I would love an input UI feature. Idk how practical it is but it'd revolutionize
.
demonnic: If I'm honest, an input UI object would be my #1 feature request for ui stuff, followed by containers bound to userWindows and being able to display the other ui objects overtop. so any UI widget can be made dockable. 🌠 in my 👀

Vadi: @kasa I think a ui input would be a downgrade, tbh. zmud-style pop-ups were annoying because, well, popups

Kasa: We're not talking about pop ups

Vadi: an input to a miniconsole would be good @demonnic it's actually not so hard to do given Mudlets architecture. A command line is attached to a console, and miniconsoles are just like a main console sans the command line.

demonnic: so could maybe give a lua function to spin up a command line with a miniconsole to attach to?

Vadi: I think more like show the command line for a given miniconsole

demonnic: ahh, miniconsole is like a console but with the commandline hidden?

Vadi: I think so.

demonnic: that -would- make it much easier

Vadi: (not spawned)

demonnic: would want to make sure input from each command line could be processed independently, ideally.

Kasa: But would the cmd line need to be reimplemented? Yeah

Vadi: for sure, would need to think on a way to handle those inputs - perhaps as events

demonnic: yeah that was my thought, when you init a miniconsole with a commandline, give it an event to fire on newline or command entry, not sure how that's tracked in the c++ code

Vadi: yes, https://github.com/Mudlet/Mudlet/blob/development/src/TConsole.cpp#L281

Vadi: if it's not a debug console and not a subconsole (userwindow/miniconsole) - spawn and attach a command line to it. So much easier to do given this design, for anyone who wants to give this a go

Kasa: Ok but does a TCL send to the game you're connected to by default?

Vadi: every game has it's own command line, otherwise you'd lose what you typed when you switch tabs

Kasa: Okay and how will that interact when spawning one in a miniconsole?

demonnic: hence the need to think of a way to handle those inputs

Kasa: Without reimplementation where will that try to go?

demonnic: but since we already determine if it's a sub window, we can use that to determine what to do with the output. I would think

Kasa: Yes I'm asking for practical reasons before I dig in the guts

Vadi: What demonnic said makes sense

demonnic: might need some adjustment in TCommandLine, it looks like right now it passes a call to the host object's send() method.

Kasa: That's what I was hoping to know @demonnic ty

demonnic: sure thing. I may take my clone and play with the idea a little bit later tonight or tomorrow. But no guarantees

@SlySven
Copy link
Member

SlySven commented Aug 18, 2018

All TConsole instances have an input/command line - it is just hidden in all but the main profile instance. Whilst it would be trivial to un-hide it in other cases a careful review of a lot of the codebase will be needed to ensure that all side-effects of doing that are taken care-of.

💡 As a preliminary to doing the above I'd strongly recommend revising the TConsole code to replace individual bool flags that partially indicate the sort of each instance with a single enum/QFlag - this would make the code cleaner and easier to review - i.e. replace the following TConsole members:

  • mIsDebugConsole - this is (pointlessly, IMHO) duplicated in the pair of TTextEdit instances that each TConsole has and is only set on the Central Debug Console instance.
  • mIsSubConsole - seems to be set if the TConsole has a parent on instantiation and is not the Central Debug Console
  • mUserConsole - only used in the TConsole::closeEvent(QCloseEvent*) handler - and set via (void) TConsole::setUserWindow() in:
    • (TConsole*) TConsole::createBuffer(const QString& name)
    • (TConsole*) TConsole::createMiniConsole(const QString& name, int x, int y, int width, int height)
    • (bool) mudlet::openWindow(Host* pHost, const QString& name, bool loadLayout)
  • mWindowIsHidden - not currently used!

with a single value that can be set on instantiation (i.e. in the constructor 😉), declared in the public part of the class like this:

enum ConsoleTypeFlag {
    UnknownConsoleType = 0,
    CentralDebugConsole = 0x1, // only one in whole application
    MainConsole = 0x2, // one per profile, obviously - also manages some aspects of subconsoles
    ErrorsConsole = 0x4, // one per profile, located in the lower right part of the Editor GUI
    SubConsole = 0x8, // multiple ones per profile with a name that is unique within all labels, subConsoles, UserWindows and buffers within that profile - is overlaid onto of the `main` instance
    UserWindow = 0x10, // multiple ones per profile with a name that is unique within all 'SubConsole's, 'UserWindow's, 'Buffer's *AND 'Label's* within that profile - is dockable around edge of main application window or is floatable - may be combined with other dockable widgets of the Mudlet application to form a tabbed arrangement {a.k.a. "MiniConsole"}...!
    Buffer = 0x20 // storage system for content not indented for immediate display to/by the user
};

Q_DECLARE_FLAGS(ConsoleType, ConsoleTypeFlag)

// Outside of the `class TConsole(...)` declaration:
Q_DECLARE_OPERATORS_FOR_FLAGS(TConsole::ConsoleType)

Also the related TTextEdit::mIsMiniConsole is also set on just the TTextEdit instances on dockable TConsoles by the (bool) mudlet::openWindow(Host* pHost, const QString& name, bool loadLayout) call and that and the code that checks for it could be handled by a direct examination of the parent TConsole "type" having the TConsole::UserWindow flag bit set - this is the same duplication as that of the mIsDebugConsole in both TConsole and the pair of TTextEdits that each has!

Updated: done, with follow up work in progress...

@vadi2
Copy link
Member

vadi2 commented Aug 18, 2018

I strongly oppose replacing simple booleans with the more complicated bitwise flags - I'm having a hard enough time dealing with the existing ones that have been added 😞 Bitwise flags do make sense when you have gigabytes of repeating data, but here we're dealing with a few booleans, let's keep the code simple and approachable.

@SlySven
Copy link
Member

SlySven commented Aug 18, 2018

What I propose (using an enum which just happens to use powers of two so that the Q_DECLARE_FLAGS() and Q_DECLARE_OPERATORS_FOR_FLAGS() both work with it) means that we would have a single member that tells anything that needs to work with the TConsole class whether they are working with:
1. the Central Debug console;
2. the main console of a profile,
3. the profile's editor errors display,
4. a console laid over the main profile's console
5. a floating/dockable window
6. a non-displayable buffer.

All of these have different code requirements and the current usage of bool type flags is messy and fragile.

Code that needs to work only on a subset of all of those can simply do an if ( (TConsole *)ptr->mType & (an OR-ed subset of the above flags)) { instead of having to pick from a seemingly random and ad-hoc collection of bool flags.

For instance, in relation to this PR, do we want to enable the command line on the errors display in the editor or within a buffer instance or the Central Debug console - NO - is there a simple/consistent way to check/test that the current TConsole is one of those - NO - whereas what I would like would be simply something like:

    if (mType & (MainConsole|SubConsole|UserWindow)) {
        layerCommandLine->show();
    } else {
        layerCommandLine->hide();
    }

I mean, look at the TConsole constructor and tell me that it is clear, easy and consistent for each of the six different usages to be setup/configured...

Updated: done, with follow up work in progress...

@vadi2
Copy link
Member

vadi2 commented Aug 19, 2018

🤔 that makes sense if it's a single state. The pain I was specifically referring to was when multiple states are being saved in a bitflag.

@SlySven
Copy link
Member

SlySven commented Aug 19, 2018

Yeah, fair enough, for instance there might be an inclination to include the docked/floating state in such an enum to distinguish between them for floating/dockable (user) windows - whereas it is clear you would want that to be flagged completely separately - I will take that on-board! 😉

@vadi2
Copy link
Member

vadi2 commented Aug 19, 2018

As soon as it becomes multi-state, it should be booleans. Bitwise flags are not simple or fun to deal with - I've given it a fair go.

@keneanung
Copy link
Member

keneanung commented Aug 21, 2018

Isn't the list above (with the exception of floating/dockable) mutually exclusive? In that case, I agree with @SlySven about using an enum (but without explicitly listing the values as powers of 2, because you shouldn't & or | them). Then the code becomes

    if (mType == Type::MainConsole || mType == Type::SubConsole || mType == Type::UserWindow) {
        layerCommandLine->show();
    } else {
        layerCommandLine->hide();
    }

I also agree with @vadi2 that as soon as we have multiple states to handle, it's better to use different members for them: One for dockable and one for the type for example. Bitwise flag sare just not very readable and have the tendencies to lump unrelated stuff together.

@SlySven
Copy link
Member

SlySven commented Sep 1, 2018

When the condition/types ARE mutually exclusive then the use of powers of two (and which then enables the use of Q_DECLARE_OPERATORS_FOR_FLAGS(...) so you can use boolean bit-wise operators - as Qt does extensively) is a very sensible choice as the original code is more compact and (to 'C' coders) just as readable IMHO...

@Kebap
Copy link
Contributor Author

Kebap commented Dec 24, 2018

Another request for this maybe? See #2146

@SlySven
Copy link
Member

SlySven commented Dec 27, 2018

Well I've done the classifying the TConsole types as #2138 if anyone wants to comment - and that has shown up some unreasonable bits of code being enabled for, particularly the Buffer type instances of it...!

@druuimai
Copy link
Contributor

just to add this we need to consider how aliases will respond to the different console/window other than the main console. maybe to add "console" so it will know which to respond or not to respond to:

if consoleName == "chat" do send("say " .. matches[1]) end --etc
Just throw my idea here.

@SlySven
Copy link
Member

SlySven commented Jan 17, 2019

Depending on what we want to allow in the mini-console / user window command line we may need a getConsoleName() - however this proposal will run into issues with which console has the keyboard input focus - as this runs in exactly the opposite direction that PR #2178 is currently trying to solve. 😖

@vadi2
Copy link
Member

vadi2 commented Jan 17, 2019

I don't see why - the usr clicks on an input line in a miniconsole, it'll get the focus. We'll be okay here!

@SlySven
Copy link
Member

SlySven commented Jan 17, 2019

But isn't that PR about making the input focus stay in/go back to the main TConsoles TCommandLine when the user has clicked on somewhere in the mini-console / user-window?

@vadi2
Copy link
Member

vadi2 commented Jan 17, 2019

Yeah, somewhere in the text part - but this input line part would be exempt :)

@Kebap
Copy link
Contributor Author

Kebap commented Jan 18, 2019

I would imagine, if I click into a text area of a miniconsole, and that miniconsole has its own custom input line, then I would expect my focus to go in that input line, not back in the main window.
But this is an edge case of an edge case and can surely be integrated later if it seems to tedious now,

SlySven added a commit to SlySven/Mudlet that referenced this issue Feb 1, 2019
Using QString::toLatin1() when transferring text to and from the Hunspell
library will miss-encode any characters that are not in the ISO 8859-1
(Latin 1) character set and will not work if used with languages that use
characters outside of that set.

The existing code only installed the user selected dictionary for spell-
checking in the constructor of the `TCommandLine` class instances which
meant that changes with the profile active did not take effect until the
next profile load - which is less user friendly than changing it when
requested. This commit corrects that problem.

At the moment there is (or should be?) only one `TCommandLine` per profile
- for the Main Console.  I have chosen to use the signal/slot method for
the `Host` class to inform the rest of the profile that the dictionary
has been changed. This is so it can be extensible in the future if
Mudlet#1897
is implemented, so that we could:
1. centralise access to a profile dictionary
2. provide a custom dictionary for each profile
3. give Lua API access to both a custom and indeed the selected dictionary

although we will probably want to consider moving the code accessing the
Hunspell library to the main profile `TConsole` class.

I have also added a qDebug() line to report on the loading of a dictionary,
during development of this PR the output from this showed me that each
dictionary could have a different character encoding and that it was
essential to use the indicated one when transferred text into and out of
the Hunspell library.

Further exploration of that library also revealed that there was an extra
return value from the Hunspell_spell(...) call. It is necessary to include
both the C and the C++ header file for it to be available. This value
indicated that the word was spelt correctly but there was some problem with
it.  I originally thought that it was a marker for obscene words and
decided to include handling that in the spell-checker. However testing
revealed that that did not appear to be the case.  The exact meaning of the
HUNSPELL_OK_WARN value is not currently clear to me, but since they make
the information available I decided to use it.  Should a word be flagged in
that way it will be underlined in a blue dash-dot line (to distinguish it
from the wavy red line that is used for misspelled words).

Remove useless cruft from class:
* (bool) TCommandLine::mAutoCompletion
* (QPalette) TCommandLine::mAutoCompletionPalette
* (QString) TCommandLine::mAutoCompletionTyped
* (QMap<QString, int>) TCommandLine::mHistoryMap
* (QString) TCommandLine::mLastCompletion
* (bool) TCommandLine::mTabCompletion
* (QPalette) TCommandLine::mTabCompletionPalette

Clean up initialisation list.

Improve the context menu action so that the spelling suggestions are
presented on top of the normal context menu rather than separately as
(before).

Fixup some CLazy else-after-return code style warnings.

The context menu spelling checker will now report "no suggestions" rather
than producing no information at all if it cannot find a suggestion which
helps to make it different from the case where the word is correctly
spelled.

Replace one use of `for (;;)` with the more obvious `forever`.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this issue Feb 3, 2019
…rs (#2269)

Using QString::toLatin1() when transferring text to and from the Hunspell
library will miss-encode any characters that are not in the ISO 8859-1
(Latin 1) character set and will not work if used with languages that use
characters outside of that set.

The existing code only installed the user selected dictionary for spell-
checking in the constructor of the `TCommandLine` class instances which
meant that changes with the profile active did not take effect until the
next profile load - which is less user friendly than changing it when
requested. This commit corrects that problem.

At the moment there is (or should be?) only one `TCommandLine` per profile
- for the Main Console.  I have chosen to use the signal/slot method for
the `Host` class to inform the rest of the profile that the dictionary
has been changed. This is so it can be extensible in the future if
#1897
is implemented, so that we could:
1. centralise access to a profile dictionary
2. provide a custom dictionary for each profile
3. give Lua API access to both a custom and indeed the selected dictionary

although we will probably want to consider moving the code accessing the
Hunspell library to the main profile `TConsole` class.

I have also added a qDebug() line to report on the loading of a dictionary,
during development of this PR the output from this showed me that each
dictionary could have a different character encoding and that it was
essential to use the indicated one when transferred text into and out of
the Hunspell library.

Remove useless cruft from class:
* (bool) TCommandLine::mAutoCompletion
* (QPalette) TCommandLine::mAutoCompletionPalette
* (QString) TCommandLine::mAutoCompletionTyped
* (QMap<QString, int>) TCommandLine::mHistoryMap
* (QString) TCommandLine::mLastCompletion
* (bool) TCommandLine::mTabCompletion
* (QPalette) TCommandLine::mTabCompletionPalette

Clean up initialisation list.

Improve the context menu action so that the spelling suggestions are
presented on top of the normal context menu rather than separately as
(before).

Fixup some CLazy else-after-return code style warnings.

The context menu spelling checker will now report "no suggestions" rather
than producing no information at all if it cannot find a suggestion which
helps to make it different from the case where the word is correctly
spelled.

Replace one use of `for (;;)` with the more obvious `forever`.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@vadi2
Copy link
Member

vadi2 commented Sep 1, 2020

@epsilon-phase @druuimai @Symple85 help test it over at #4055 (comment) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants