IRC client update of UI and logic#1072
IRC client update of UI and logic#1072itsTheFae merged 8 commits intoMudlet:developmentfrom itsTheFae:update-Irc3
Conversation
This retains the primary functions of the old IRC client but adds many more new IRC commands by default. The UI is slightly more organized and interactive now.
Input History stores a total of 8 previously entered commands the user can cycle through using the Up Arrow key. The previously include /msg command has been added again. Colors of some messages have been updated for (relatively) easier distinction of message lines.
I have no idea what i'm doing wrong and don't understand how to do it right. Including these changes in hopes that it lightens the work load on someone else anyways.
|
Awesome! Thanks for this. Eyeing our resident CMake expert @keneanung for help here 😇 |
|
Looking at the logs it is now clear that that last commit of "Half-done CMake stuff..." is an accurate assessment - the QMake builds all completed but all the CMake ones crashed and burned (e.g. the Linux / Clang / CMake one):
🤔 I wonder can we get a |
|
I'll try to have a look tonight, but no promises. |
|
I've tried a good number of things to get this working but nothing seems to be working so far. As far as I can tell, the |
|
To give an update about this: I managed to clear out the issues regarding compiling the communi library itself, but linking it into the mudlet executable still fails using cmake. The progress is slow because I don't have my dev machine available right now. See the current WIP at https://github.com/keneanung/Mudlet-1/tree/update-Irc3?files=1 |
Things needed to fix CMake builds: - communi CMake builds needed `include_directories` explicitly specified - more MOC calls in communi - more explicit dependencies to MOC files in communi - ircmessageformatter must be compiled into mudlet
|
@itsTheFae Your turn again cmake works now! |
|
Thanks for the help with this! The new client should be stable and ready for any extra testing and review before merge. I'll be opening a second PR with the lua additions soon. |
vadi2
left a comment
There was a problem hiding this comment.
Haven't finished reviewing, but I've gotta go - just have some minor comments so far.
Overall, I tried it out and stuff works, so that's awesome!
| connection = new IrcConnection(this); | ||
| connection->setReconnectDelay(5); | ||
|
|
||
| lineEdit->setAttribute(Qt::WA_MacShowFocusRect, false); |
There was a problem hiding this comment.
Would you add a "why" comment in here? 'cause I've got that question.
There was a problem hiding this comment.
I don't think it's needed. It was copy pasta from the simple client example that got left as I wasn't sure if it was needed for some reason due to the focus policy that is set on the text message display and the input field.
| QShortcut* shortcut2 = new QShortcut(Qt::Key_Up, this); | ||
| connect(shortcut2, SIGNAL(activated()), this, SLOT(slot_onHistoryCompletion())); | ||
|
|
||
| connect(lineEdit, SIGNAL(returnPressed()), this, SLOT(slot_onTextEntered())); |
There was a problem hiding this comment.
Did you know you could use compile-time signal connections? That way any errors are discovered when you're compiling and not at runtime - it's safer. I think if you right-click on this in Qt Creator and select refactor, it'll give you the option to convert.
There was a problem hiding this comment.
I did not know, I'll check into this.
| connect(connection, SIGNAL(partMessageReceived(IrcPartMessage*)), this, SLOT(slot_partedChannel(IrcPartMessage*))); | ||
|
|
||
| // TODO FIXME : This needs to be updated to use methods provided by Host. | ||
| qsrand(QTime::currentTime().msec()); |
There was a problem hiding this comment.
Already covered by https://github.com/Mudlet/Mudlet/blob/development/src/main.cpp#L344 ! No need to do this.
There was a problem hiding this comment.
Will be removed next push.
| connection->setHost("irc.freenode.net"); | ||
| connection->setPort(6667); | ||
| mNickName = nick; | ||
| mUserName = "mudlet"; |
There was a problem hiding this comment.
Do mUserName = QStringLiteral("mudlet"); - it then makes it so the QString is constructed at compile-time instead of runtime. Pretty cool!
Same thing for all other places you've used raw string literals.
| ircBrowser->append(IrcMessageFormatter::formatMessage(tr("$ Host: %1:%2").arg(mHostName, QString::number(mHostPort)))); | ||
| ircBrowser->append(IrcMessageFormatter::formatMessage(tr("$ Nick: %1").arg(mNickName))); | ||
| ircBrowser->append(IrcMessageFormatter::formatMessage(tr("$ Auto-Join Channels: %1").arg(mChannels.join(" ")))); | ||
| ircBrowser->append(IrcMessageFormatter::formatMessage(tr("$ This client supports Auto-Completion using the Tab key."))); |
There was a problem hiding this comment.
To be honest completely missed this message because of the freenode spam, only saw it now reviewing the source code.
There was a problem hiding this comment.
Yea the config stuff isn't really needed. I was thinking of adding the other information to the /help command instead of where it's at now.
| break; | ||
|
|
||
| const QString cmdName = QString(cmd->parameters().at(0)).toUpper(); | ||
| if (cmdName == "CLEAR") { |
There was a problem hiding this comment.
How come we can't keep the nice switch statement in here?
There was a problem hiding this comment.
I had some reason for it but I know it wasn't a good one.
There was a problem hiding this comment.
Actually was because QString is illegal in switch statements.
The only way to know what command you're actually handling when dealing with the IrcCommand::Custom type is to inspect the command string itself for the base command name.
There was a problem hiding this comment.
dlgIRC::processCustomCommand(IrcCommand* cmd) is only for custom commands. line 205 checks cmd for type IrcCommand::Custom and if it is not that type the function returns true allowing the rest of the sending process to take place.
| mInputHistoryIdxCurrent = mInputHistoryIdxNext; | ||
| ++mInputHistoryIdxNext; | ||
|
|
||
| qDebug() << "History Count:"<< mInputHistory.count(); |
There was a problem hiding this comment.
Thought I'd removed that already, will be removed in the next push.
|
How about use switch for everything not custom, and if match custom, use if
inside that?
…On Mon, 12 Jun 2017 6:57 am Kae, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/dlgIRC.cpp
<#1072 (comment)>:
> }
- case IrcMessage::Type::Notice: {
- IrcNoticeMessage* rmsg = static_cast<IrcNoticeMessage*>(msg);
- irc_gotMsg(rmsg->nick(), rmsg->target(), rmsg->content());
- break;
+
+ const QString cmdName = QString(cmd->parameters().at(0)).toUpper();
+ if (cmdName == "CLEAR") {
Actually was because QString is illegal in switch statements.
The only way to know what command you're actually handling when dealing
with the IrcCommand::Custom type is to inspect the command string itself
for the base command name.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1072 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAGxjFPTuHixuvQiILLsdhmITVJ9f95Yks5sDMVIgaJpZM4NwgcG>
.
|
|
I have made several of the changes noted, but they are currently in the second PR branch (not yet pushed to remote.) I can backport them to this PR if needed. :) |
|
It is fine with me, no need. |
|
I started to compile this and got some unexpected warning messages at the beginning:
Those Update: it does not seem to have prevented the build of an executable application... 😖 |
|
There is a regression or a change of behaviour with this PR. Prior to this PR it was possible to activate the IRC help-chat BEFORE a profile was started - and I could see that being used by someone who has a range of characters saved under different profiles to play on different "sides" in a MUD - they may wish to logging onto an IRC Server to see which side their "mates" are playing on before starting to log on with a particular profile. This now prevents them doing so as it stops IRC working before a profile has been loaded. This may not be desirable because some MUDs require a time delay (and impose penalties for not complying with them) after closing one profile on one side before you start another profile with a "character" on a different side. {I speak from experience - WoTMUD requires a a delay of at least 5 minutes IIRC before logging in as a player on a different side!} If IRC is used to coordinate grouping up like this then linking IRC to a profile is not going to be useful 😒 for such users. If this change is going to be retained then we should add the "IRC" main toolbar button to those which are disabled before a profile is loaded {see and revise |
|
👎 This PR is not respecting the previous application-wide "nickname" setting held in the base ~/.config/mudlet/irc_nick file 😠 - instead it is only respecting a per-profile setting. Given that I was someone who HAD made such a setting in the past I would expect it to have been respected (and imported by default) perhaps with a tool-tip advisory that it is now "per-profile" - and provision to specify that application wide setting back in the place it was previously on the profile preferences dialog and advice that one could set a profile specific one on the IRC tab...! |
|
👍 Though I do like the ability to finally be able to close an IRC session without killing Mudlet itself - before it did not seem possible. |
|
I hadn't considered that aspect. It would only make sense to keep the client available even while the default host is loaded. I am not sure about Lua interaction with the client opened in the default profile. Should that client be invisible to Lua? It would get confusing with two clients posting into any profile without some added code. I can also add a function to inspect the Application Wide settings file and import its value by default, if the new file does not already exist. |
💭 We need to get the semantics right, previous users of the IRC facility will possibly have an application wide setting (like I have) and in some situations I guess it is useful to have a common "handle" no matter which "profile" is in use. Multi-playing is a minority MUD area I think but I think there is scope to use a I am still not entirely sure how much of a profile the "default_host" actually represents - it does own the central debug console and it does seem to be a recipient for "global events" - and I guess it might be used as the basis for a new profile for the user when they click on "new" (or "copy") in the connection preferences dialog but I am not entirely certain about what characteristics and objects that it otherwise possesses. 😕 |
|
Having two fields where you can set the nickname is silly. How about when you create a new profile, the default nickname is set to that of the last used profile instead? |
|
The functionality I'm currently using just attempts to load the old file if it exists and a new name file hasn't already been created. |
|
OK, why not remove the hassle altogether and when a new profile is created, copy in the last-used's nickname file in? That way you'll get a consistent nickname if you create more profiles that you still have the option to customise as needed. |
|
I'm not sure it's hassle, was originally to import the old nickname config and not to act as an app-wide default. However seeing there is a desire for that functionality I have opted to simply write any nickname update to both that original config (the "app-wide" default IRC nick file) and the new configuration file which is stored in each profile. |
SlySven
left a comment
There was a problem hiding this comment.
I'm not sure the Nick-name stuff is quite how I expected it to be - with the code here and a previously stored "master" IRC Nick that is always used - but as you can only have one IRC window open at a time different Nicks at the same time in multiple windows is not something that can happen. The intricacies of this currently escapes me but the important thing is CMake building works for me on Linux so we'll see how things work out on the linked to successor PR...
|
@vadi2 I trust you know what happens to this next (whether anyone else reviews it or it gets merged in...) |
|
Awesome, thanks @SlySven :D @itsTheFae feel free to merge, this one's good to go! |
* Add Communi Model & Util libs * Full refactor of IRC Client code & UI This retains the primary functions of the old IRC client but adds many more new IRC commands by default. The UI is slightly more organized and interactive now. * Adds QPointer to IRC Dialog pointer variable. * Update Lua `sendIrc()` slightly. * Added /msg command, Input History, and updated display colors. Input History stores a total of 8 previously entered commands the user can cycle through using the Up Arrow key. The previously include /msg command has been added again. Colors of some messages have been updated for (relatively) easier distinction of message lines. * Updated channel handling. * Fix CMake builds Things needed to fix CMake builds: - communi CMake builds needed `include_directories` explicitly specified - more MOC calls in communi - more explicit dependencies to MOC files in communi - ircmessageformatter must be compiled into mudlet
* Removed duplicate clear event handlers call * Revert back to dev version. 3.3 here we go! * Adds saveWindowLayout() call to AutoSaveOnExit processing. * Edbee editor implementation (#985) * Added a frame to edbee so it looks nicer * Got utf8 module to load (#1074) (update packaging!) * Modified tooltip for Save and Save Profile to clearly indicate their keyboard shortcuts * Added: shortcut for delete action in editor (#1084) * shortcut added for delete action * Removed redundant edbee config related code (#1091) * Delete TScriptEditorManager.h * Delete TScriptEditorManager.cpp * BugFix: import some fixes by "Zolder" from Mudlet (Realms of Legends) fork import: some Zoilder fixes from the MudletRL fork: * in TBuffer::translateToPlainText(...): make some MXP related code execute only if Host::mFORCE_MXP_NEGOTIATION_OFF is NOT set as well as another condition. * in TBuffer::wrap(...): change a for(...) {...} loop termination condition be a "<" rather than a "<=" test - which sounds like an "off-by-one" error fix-up. * In TTextEdit:updateScreenView(): revise code that seems to prevent the width of a "main" console being set too small and NOT written to the "parent" Host::mScreenWidth variable - the original modification removed such a restriction completely, but I have just lowered the minimum from 100 to 40 and ensured it is always stored both in the local "main" console and to the parent record in the Host class for that console. * In TTextEdit::drawForeground(...): revise the behaviour *I think* when at the point where a console "split" becomes unnecessary... Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Cut down on the length of some variable names (#1086) * Clarified use of QLatin1String * Adding GCC 5 for C++14 requirement (#1098) * Fixed "reset map colors" button to work (#1049) * Fixed "reset map colors" button to work That's a 5 year old bug out of the way! * Change travis configuration to use installed gcc-5 (#1108) * Change travis configuration to use installed gcc-5 * Fixes #1104 (#1109) Thanks to Rick for his prompt response on this * Fixed typo in GeyserLabel.lua Per #1112 * Fix locale to C Enforce uniform locale so scripts don't get tripped up on number representation differences (. vs ,) * IRC client update of UI and logic (#1072) * Add Communi Model & Util libs * Full refactor of IRC Client code & UI This retains the primary functions of the old IRC client but adds many more new IRC commands by default. The UI is slightly more organized and interactive now. * Adds QPointer to IRC Dialog pointer variable. * Update Lua `sendIrc()` slightly. * Added /msg command, Input History, and updated display colors. Input History stores a total of 8 previously entered commands the user can cycle through using the Up Arrow key. The previously include /msg command has been added again. Colors of some messages have been updated for (relatively) easier distinction of message lines. * Updated channel handling. * Fix CMake builds Things needed to fix CMake builds: - communi CMake builds needed `include_directories` explicitly specified - more MOC calls in communi - more explicit dependencies to MOC files in communi - ircmessageformatter must be compiled into mudlet * IRC3 with more Lua features (#1087) Changes IRC client to run with settings given per-profile, making the IRC client more visible and configurable within Lua. This adds configuration options to the settings window in the "Special Options" tab for configuring an IRC client on the host profile which opens the settings window. See the PR summary comment for details on available Lua functions and IRC Client commands. * Add a Code of Conduct (#1110) * Create CODE_OF_CONDUCT.md * Fix for the QMake warnings caused by the IRC library includes. * Finished formatting all filed & added braces (#1115) * Finished formatting all filed & added braces * Re-enabled map load on profile load (#1118) * Re-enabled map load on profile load - disabling it broke scripts that relied on map data w/o the map open * updated assignments. * Event-based gamepad functionality (#1106) * Add theme switching to code editor (#1095) * Fix more Toolbar & Button bugs (#1080) (#1090) * Fix bugs with Buttons & Toolbars. - Fixed change detection for location, orienation, and CSS data. - Buttons are not rendered unless they are part of a group, Toolbar/Menu. - ToolBars whos locations are changed now hide their previous TToolBar or TEasyButtonBar instance as needed. * Fix Buttons being left behind when parent toolbar is deleted. Also allows TAction to use QPointer<T> * Update old sourceforge page links (#1132) (#1134) * Fixed blank command separator to not separate as people expect * Replaced isTempKey, isTempAlias, etc with isTemporary (#1102) * also pushed isFolder() and setIsFolder() to the parent Tree class * Give meaning to encodings (#1107) * Delete dev_README Deleted outdated dev_README file. It also didn't seem to be used all that often, else people would have raised the fact that it's outdated... * Update travis to use Qt 5.9 for compiling (#1141) * Re-add single QT 5.6 build job Since we want to keep sure that the minimal Qt version is still compilable (currently we need at least 5.6), we add a single job for that version. * Refactor: rename QMake project/sub-project files (#1103) * refactor: rename QMake project/sub-project files It is down right confusing having several project files in the Qt IDE which are all labelled "src" - this commit renames the main project file from "src.pro" to "mudlet.pro" and also renames: * the lua_yajl module from src.pri to lua_yajl.pri * the luazip module from src.pri to luazip.pri it also: * adds a new communi.pri file (see below) I also took the liberty of renaming the project resource file from: * mudlet_alpha.qrc to mudlet.qrc (we are well past the alpha stage now!) and sorting a couple of items into the right order. By creating a local project file 3rdparty/communi/communi.pri it is possible to group the three separate but related communi libraries into one - as it happens the effective content of the new file is identical to that of the communi libraries own libcommuni/src/src.pri but it has some notes on how the portion of the upstream library that we are using was picked out of there; how to identify the effect version and how to (hopefully) recreate things if upstream gets revised in the future! By not using the upstream "src.pri"/"src.pro" files we avoid the confusion of having a generic "src" project in the Mudlet project as a whole and it is clearly identify-able as a file from the Mudlet project rather than the sub-project that it pulls into the whole thing... There is an minor nuisance side-effect in that having the main project src.pro file open in a Qt Creator IDE or stored in a session will get confused when this commit takes effect as the file will disappear from the repository - it will be necessary to close the project and open the new src/mudlet.pro project file (and possibly re-enter the build steps) - OTOH the main project will now be identified as "mudlet" in Creator rather than the previous "src". 😄 Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Removed accidental debug echo left in (#1151) * initialize int nIrcPort with dlgIRC::DefaultHostPort value. (#1153) * Removes false from sendIrc() return, ignores command filtering status (#1154) * Removes false from sendIrc() return, ignores command filtering status (not used). * Remove punctuation from lua messages * Improved layout of IRC preferences (#1152) * Fix get map events (#1120) * Adapt addMapEvent error messages to current standard * Fix getMapEvents() The function now returns a proper indexed table of objects that each describe a map event. * Add missing call to set Editor as editable. (#1159) * Fixed tab indent to be two spaces again * Fixed getComputerEncoding in case no key found It'll now return the original value passed - it's way better than returning the wrong value. * Add linux ci deployment (#1128) * Modify files needed for CI deployment of linux builds * Syntax fix in .travis.yml * tar the CI AppImages as well * Add lua sqlite runtime dependency * Unset LD variable for linuxdeployqt * Bugfix: unset LD_LIBRARY_PATH in every codepath * Test: install appmenu-qt5 for global menus * Revert 781ef97 (Install appmenu-qt5) The issue was that the first AppImage used Qt 5.6.2 instead of Qt 5.9 (which is how 3.2 was built and how it is build now after merging development in) * Remove manager again (#1163) * Improved map load output to be condensed down if it's all successful (#1117) OK! Would be happy to see that in as well for maps that take a while to load (>1s). * Re-enable search highlighting (#1096) * Remove mainwindow statusbar (#1162) * Replaced statustips with tooltips in the mapper menu * Removed statusbar * Tagging 3.3.0 * Initial QTextToSpeech additions * Further TTS additions and bugfixes * Wrapping TTS functions in #ifdefs for QT_TTS_LIB * Missing #ifdefs * More misplaced #ifdef lines for TTS functions * More QT_TTS_LIB ifs * Adding QTextToSpeech to cmake lists * Various TTS updates * Adding GetState to TLuaInterpreter.h * Minor fixes * Tts temporary fixes * Exchanging toLatin1 with toUtf8 * Fix cmake TTS module includesion * Install Qt 5.9 TTS module on linux * Use Qt provided definition for enabled modules We don't need (and possibly don't want) to provide our own definition for when a module is enabled. Let's use the Qt provided definition instead. * Output confirmation messages of enabled Qt modules * Use upper case "Gamepad" in output * ttsClearQueue crash fix * Fix for global speech events * Whitespace replacement * Various minor fixes * More minor fixes. * Cleanup: make CMakeList.txt have uniform indenting and adjust CMake message The minimum versions of the Qt TextToSpeech and Gamepad reported in the CMake project files needed tweaking. I have also tried to regularise the indentation in the Mudlet CMake project files to standardise on 2 spaces. Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Reformat: clean up code-loyout of new TTS TLuaInterpreter code Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * BugFix: fixup a switch() in (int) TLuaInterpreter::ttsGetState(lua_State*) This switch was missing "break;" on all the cases and must have been defective without them - for all but the last case extra strings would have been pushed onto the lua stack but as the return value was 1 they may have been discarded... Signed-off-by: Stephen Lyons <slysven@virginmedia.com> * Enable Qt 5.9 Windows builds for testing * Deploy Qt 5.9 builds for testing * Fixing double-events for queueing purposes * Added missing symbol * Added missing symbol * Updating tts events to globals and adding voice * Create TTSValues.lua * Adding TTSValues to LuaGlobal * Adding TTSValues to Mudlet project * Update CMakeLists.txt * Update TTSValues.lua * Update Qt to 5.10 for trusty This is needed to hopefully have some TTS backend plugins in Linux * Queueing and crash fixes * Renaming tts funcs * Renaming internal functions * Adding functions for retrieving settings * Fixing typo * Making ttsGetState public * Improve error messages for ttsGetCurrentLine() It can be incredibly frustrating to just get a boolean back as an error message when you don't know what is going on * Update error message to be consistent with the new style * Aligned error messages to be consistent with the new style * Fix ttsClearQueue to return nil+error, not actually error on invalid index * Minor code formatting * Floats to doubles * Update documentation links
This PR provides a complete rewrite of the IRC Client UI and logic, as well as the addition of
IrcModelandIrcUtilcomponent libraries of Communi.These changes attempt to closely replicate the functionality of the old client so as to avoid breaking expected functionality, however this PR is also a precursor to the changes made in this branch: https://github.com/itsTheFae/Mudlet/tree/feature-Irc3-WithMoreLua
That being the case, my primary focus with this PR is getting cmake to play nice with the extra libraries needed, since I couldn't manage to do it on my own. Anything outside of that can probably wait and be addressed in the second PR with the functional changes and lua additions for the IRC client.