Enhance add multi presence support #11
Conversation
The include files are identical and the OSes know which libraries to load so no need to keep completely separate file-trees for each one's files. Signed-off-by: Stephen Lyons <[email protected]>
Also adds WoTMUD to the registered MUDs so that it's icon is used if "wotmud" is the game set. Signed-off-by: Stephen Lyons <[email protected]>
There is an alternative Discord_Initialize function that does not have a SteamID which is better suited to Mudlet's use. (We are not a Steam game) Signed-off-by: Stephen Lyons <[email protected]>
Signed-off-by: Stephen Lyons <[email protected]>
Slightly incomplete, loading/saving of the presence with the profile needs clean-up but the lua command to set it and use it works... Signed-off-by: Stephen Lyons <[email protected]>
|
I've subsequently found that it won't now compile cleanly on my FreeBSD machine so I will have to add another commit to exclude bits for x86-Linux and FreeBSD platforms... |
src/TLuaInterpreter.h
Outdated
| static int getDiscordLargeIcon(lua_State*); | ||
| static int getDiscordLargeIconText(lua_State*); | ||
| static int getDiscordSmallIcon(lua_State*); | ||
| static int getDiscordSmallIconText(lua_State*); |
There was a problem hiding this comment.
Why these functions duplicate the API I've already got?
There was a problem hiding this comment.
The setXxxx ones work directly on the Rich Presence entry directly and will work with any Presence Id - whereas your fancy 😄 ones can only work with the Mudlet one (which requires, for instance, that there is an icon with the same name as the MUD and that is what is used for the Large icon) - and now throw an nil+error message if that is not the one in use for the given profile.
There was a problem hiding this comment.
I have some more commits pending that:
- adds the needed conditional compilation for those cases where Discord is not available (32-bit Linux - e.g. x86 Debian; FreeBSD)
- MIT license entry in the
About Dialog - fix a bug where I'd forgotten to save the entered Presence Id to the profile
- fix a bug where the initial opening of the profile when another Presence Id was to be used for that profile was not initializing the remote Discord RP with that Id until the first change was made to any contents.
I have to run them past a Linux platform as well as the Windows one that I was using and check that FreeBSD still works - the last two are okay right now but I hit something late last night and need to resolve it but have some RL stuff I MUST do this morning.
There was a problem hiding this comment.
We can't have two APIs that duplicate each other, that'll be confusing for users and not really smart on our part.
There was a problem hiding this comment.
We'll have to merge them somehow - make the existing API work with any presence ID and add the getters.
There was a problem hiding this comment.
I'll busy today so I can't think on a solution for this, could you think of one?
There was a problem hiding this comment.
Your solution is predicated - not unreasonably BTW - on a single large icon for each Mud that Mudlet knows about and has entered into the QMap<Host*, QString>mGamesNames and which is selected when the lua function setDiscordGame is used - however should a MUD want to have a range of icons they cannot do that within your API. Also reporting Playing Mudname in the detail text is not useful when the line above that will already say Mudname in bold - whilst I have tentatively changed it to say via Mudlet (connection state) that is entirely usable to say something else (of course we hope that a Mud Server Discord package might at least mention us but they would not have to...)
💡 It might be possible to replace your four C++ coded functions to ones in lua that call the lower level ones that I have produced - indeed that would mean I could simplify mine... how would that work with the gmcp and other OOB protocols that you are looking to tie into this?
There was a problem hiding this comment.
It would work fine, the game would just transmit the Discord ID as well.
I also think that it's more intuitive to use setDiscordCharacterIcon as a name than setDiscordSmallIcon, it makes more sense no?
There was a problem hiding this comment.
Only if you are using the Small Icon for that purpose! Me and Austin (an Immortal of the MUD Server + the Discord Guild owner) on WoTMUD are planning to use the Small Icon to show the Clan affiliation of the character - which would be not used for unclanned and those going incognito. Other MUDs might want to use it to represent what the character is currently doing...
On the whole, for a generic (raw) interface I think a neutral (in this case descriptive) term is a better bet. OTOH For a lua script wrapper that handles things for a specific setup and calls the raw functions, like I think we can convert your API work so far to, it would not be unreasonable to call your function that as part of a cooked interface.
"It would work fine, the game would just transmit the Discord ID as well." - I think I left a commented out suggestion for that in the GMCP code - but I do not know the intricacies of those OOB protocols so cannot be certain it is correct.
There was a problem hiding this comment.
Yeah, you can still use setDiscordCharacterIcon for any icon, it's not like Mudlet is going to validate that the image represents a character 🤣
Okay, let's drop that API though, we'll also have to change the privacy settings to reflect it. Lets go with small icon / large icon and so on.
…atforms In order to isolate the code from Platforms that do not support Discord (currently FreeBSD and 32-Bit Linux) I needed to be able to exclude some parts of the code - and it then made sense to use the same system as has been adopted for the Ubuntu (and other) fonts and the Updater. This will prevent inclusion of the Discord code if the Environmental variable WITH_DISCORD is set to a case insensitive "no" during compilation. Attempts to make CMake work in this manner was initially tried on Windows but it seems that the CMake project file is defective in other areas. Signed-off-by: Stephen Lyons <[email protected]>
This is needed because we distribute the run-time libraries ourselves as part of the source tarballs. Signed-off-by: Stephen Lyons <[email protected]>
Also: * Add/revise tool-tips for Discord controls on Preferences. * Trim leading/trailing spaces from Discord Presence Id input points. * Ensure that starting a profile with a non-default Discord Presence Id causes that data to be sent to the Discord class to be used before any Lua Mudlet events that might include Discord RP code is run. * Clean up some odds and ends: e.g. some comments and blank lines. Signed-off-by: Stephen Lyons <[email protected]>
|
I think this is largely functional now - i.e. if you are willing to merge it in so that others can consider the whole thing back in the main Mudlet#1716. |
CMakeLists.txt
Outdated
| # WITH_DISCORD is an environmental value/variable (so could be a number, a | ||
| # string, something else or not even exist). | ||
| set(DISCORD_TEST $ENV{WITH_DISCORD}) | ||
| if((CMAKE_SYSTEM_NAME STREQUAL "Linux") OR (CMAKE_SYSTEM_NAME STREQUAL "Windows") OR (CMAKE_SYSTEM_NAME STREQUAL "Darwin")) |
There was a problem hiding this comment.
I don't want this additional complication in the Makefile and everywhere in the code, I made the Discord support be automatically picked up for this reason - if it works, it'll work, and if it doesn't work, it gets turned off.
There was a problem hiding this comment.
What header file should a FreeBSD build include? Forgive that error - of course it will be the same as every other one as it is a single common file now.
OK, I'll take another look to see if I can get back to just the dynamic loading, and have a look as to why it wasn't working for me on that OS.
To better cope on those platforms where the Discord API will not be functional the lua functions now report that it is not available whereas in former version the error message passed to the caller was: "Discord API is disabled in settings for privacy" which is now only used when that is indeed the case. Signed-off-by: Stephen Lyons <[email protected]>
|
OK, have undone all that conditional compilation. And it still works. 😌 now on FreeBSD. One thing that was confusing was that I was getting lua error messages So this is now functional... |
|
Awesome. Okay lets figure out the new API then, a single one, and how it can relate to the GMCP/MSDP integration as well. MUDs having their own icons is more complicated, but a superior experience in the end so I think we can cut out the setGame stuff and so on. Will do more thinking on it. |
|
See #11 (comment) |
|
You know I am getting more attracted to Discord as we go on, and remember how reticent I was about it at first! 🤷♂️ |
|
"Awesome. Okay lets figure out the new API then, a single one, and how it can relate to the GMCP/MSDP integration as well." Would you be willing to pull this into your PR as it is now then we will have a test-bed (and CI builds) to compare and contrast a further PRs onto it as I pull out your C++ API and make the bits I put in simpler and somehow we put together a |
|
DO you want to merge this into your PR - and I can submit new stuff (sans your |
|
I don't see a need for a A little bit of upfront communication would have helped avoid this problem... But now that we have it lets resolve the issues in this PR, I don't want to be resolving issues in the main PR because as far as I can see that is ready to go in its current state. |
|
Well, the I suppose it would be possible to prevent some features from being activated {show character name, icon, server Url} if a profile using them is copied (so that duplicating a profile to one with a different name would not copy the enabled features but instead by comparing the |
|
But why would we have setDiscordGame, setDiscordArea, setDiscordCharacter and setDiscordCharacterIcon defined in Lua when the problem is that we have duplicate functionality for doing the same thing? Moving it out to Lua would still give the users duplicate functionality. Let's remove it entirely, that solves the problem. |
|
🤕 humm what do we do with the four functions you have put forward and coded though? Once we commit the main PR to the development branch we cannot take them away again and as things are with this PR the current line up is:
as it stands, my proposal adds:
with these setters
and getters:
One observation is that the form of all those setters and getters is the same so we could reduce the number of functions needed back down to 8 by replacing them with a pair: |
Delete them
No, better this way for autocompletion and documentation. |
|
In response to your last post that I didn't see until after my last one: The issue is not quite one of duplication, your code does work but, because it does several things {and not unreasonably (I can't say they are unreasonable - I was one of those asking for them!) applies some user permissions before some of those things because they could reveal more detail than the user or the MUD they are playing would want} and is tied to one method of using the Discord RP. The additions from me can do those things but is not tied to that model (one icon for the Mud for the Large Icon based on a list held in Mudlet if the game is a known one and putting the image otherwise destined for the Small one there if it is not; putting the Mud name and optionally the URL in the Large icon; calling the Small icon the "Character" unless it is made into the large one because the given value for that does not appear in a list). |
|
We can't have these two different models from two different coders confusing our users and being a headache to ourselves because people are confused. Like I have been banging all along, we have to merge them together, or get rid of one. I'm not sure how to say this otherwise, I've stressed it a lot already. 😞 User permissions must be there with either model that is used. |
|
🗡️ 😊 I am sorry I did not realise you were giving me permission to nuke your lua ones. What about the OOB code? Though I guess that will be up to the MUD Server operator to make sure they do not do anything stupid with the OOB data they send... |
|
The OOB code should be translated to use your functions. |
|
Okay, I'll see what I can do - I am sorry if this is causing anyone any stress, but I guess it is better to get this sorted out before we commit stuff into the wild... |
|
Yep. The privacy toggles should also be changed to small text / large text and so on, so the user has the option of blocking OOB data (or Lua data from a 3rd party script) from being displayed against their will. |
|
Re: Improve readability for the username pinning tooltip i.e. d946773 This makes the tool-tip for the discriminator (the 4 digit number appended to the user name) read: this seems confusing because the number is handled separately by Discord but the users may not realise it - the wording "this Discord username" just does not fit right here, IMHO. Also the tooltip is a little miss-leading I think , in that: if either the username OR the discriminator are provided then as soon as Mudlet works out what is being used (it seems to happen very quickly) and finds out that there is a miss-match then it will stop any further settable actions for this profile (although the current values remain readable). Drifting a little off-topic but IIRC from testing, having a mismatch does indeed stop the RP from being setup/used for this profile but if multi-playing then other profiles that do not have a mismatch will function as normal - indeed that is one of the goals of this, switching between two profile's one with a match and one without will show the RP for the matching Profile as if it was the only profile, though, admittedly, only when it is the active one will changes it makes be actively updated to the RP. |
|
Reviewing the revisions you have made to the Profile Preferences controls which has them say "Hide Xxxx" doesn't actually match up I think to what the flags are doing currently. They do not hide the individual parts of the RP - they just prevent the MUD server controlling them, as they were (at least as I had it) still accessible (provided the Lua access control was enabled) to the Lua system and could be controlled by that means. So, saying that they "hide" the parts of the RP actually means (or at least, I think it does) "hides from control over GMCP" currently. The checkbox "Disable game Discord Info" is still a master control but because it has been demoted from a groupbox around the other controls it is not at all obvious that if it is checked then the other controls have no effect... 🙁 |
Could you make a PR for .clang-format to discuss that separately? |
I'll go with my original solution: per-option privacy toggles apply to both server and Lua side. |
I intend them to be a hide from everything, including Lua. |
That's not true - as the name implies ( |
vadi2
force-pushed
the
Enhance_addMultiPresenceSupport
branch
from
d6e375d
to
0437f06
Compare
Will PR it separately
vadi2
force-pushed
the
Enhance_addMultiPresenceSupport
branch
from
0437f06
to
5256ff7
Compare
We don't chat in Discord presences but in Discord servers
This reverts commit 6122ebd.
What you register in Discord is an application, not a presence. Application is also a much more intuitive name.
Detail and state can't be anything other than text.
|
While working on this, I realised we can have a privacy leak where the user has disabled a certain server function (like the detail) but the Lua API can still read it - so scripts have access to this info. I'll expand the privacy options to cover the get* functions. |
|
What is the idea behind all of the complex engineering in |
|
Please direct all future discussion to Mudlet#1716. |
Amalgamation of @SlySven's and @vadi2's work that allow the game to use their own application ID for the rich presence. Adds Lua getters/setters for all Discord functionality.
Changes the following things in the GMCP spec (spec already updated):
applicationfield toExternal.Discord.InfopresenceidinExternal.Discord.Info, the server SHALL upload an icon of their logo namedserver-iconin their art assets