Wieloswiat / nodebb-plugin-ws-dice

Dice throws integrated with timeline
MIT License
2 stars 0 forks source link

Work on chat ? #78

Closed DroidBV8 closed 2 years ago

DroidBV8 commented 2 years ago

Hi,

Is it possible to make it work in the chats or in the global chat plugin?

Thank you for your answer

oplik0 commented 2 years ago

It should be possible to add this functionality using filter:messaging.save and Messaging.addSystemMessage instead of topic events, so if I have some spare time I might try doing this.

DroidBV8 commented 2 years ago

great very good news ;) can't wait to see this

Why WS ?

oplik0 commented 2 years ago

For Wieloświat (This plugin was created for use on https://wieloswiat.pl) - is just more problematic, and two letter specifier is more, well, specific than just a w (+there were some other plugins with similar 2 letter convention).

As to why add that specifier at all - there already is (or at least was) a nodebb-plugin-dice package. I prefer this kind of namespacing than adding some -alt, -2 etc. to the end.

DroidBV8 commented 2 years ago

Ok good. Thanks for your responce :)

Can't wait to see it working in global chat/nodeBB chat. That would be really cool ;)

Do you plan to keep it at the same time as NodeBB if necessary or offer it to the NodeBB community?

bye

DroidBV8 commented 2 years ago

any news my friend ?

oplik0 commented 2 years ago

I was busy last weekend, and as I mentioned - this was mostly waiting for me having some free time :)

It will work on chat now, though without images (even system chat messages seem to be protected from XSS, which is a good thing but means I'd have to work around filtering to get them to work, and I can't be bothered tbh)

DroidBV8 commented 2 years ago

oh wow OMG @oplik0 !!!

It's already excellent!!! A big thank you to you and take your time to improve it if you wish ;)

EDIT:

Maybe a small bug or not ?! We do not see the command (/roll xdx) launched by the user in the topic but only in the answer below.

image

DroidBV8 commented 2 years ago

@oplik0

It's possible to display the /roll command on the post topics like previous screenshot ? It's odd to see a blank post ;)

Thanks

oplik0 commented 2 years ago

Hmm... Yes, as I said this plugin was made for use case on a specific forum and I think this issue didn't really occur often there (most posts with rolls had context for them, so I preferred hiding the commands). I can add a setting to always show the command or perhaps just detect otherwise empty posts specifically - whichever you prefer.

DroidBV8 commented 2 years ago

@oplik0

yes !! Maybe can be detect if the post only contains the command.

--> If yes, we display the command --> If not, we do not display it.


Otherwise, I have seen this bug display:

image

And how to use /roll 4d6-L or /roll 4d6-H ?

image

Thanks man, for for your availability, you are awesome :)

oplik0 commented 2 years ago

Ehh... > is the result of sanitization again, replaced them with actual characters :)

As for the -L - it appears I forgot to update README to fit notation changes in the rpg-dice-roller package I'm using for parsing dice notation, since they changed the drop notation some time ago. See here for the current one: https://dice-roller.github.io/documentation/guide/notation/modifiers.html#drop-d-n-dh-n-dl-n But TL;DR: append dl1 to drop one lowest, dh1 to drop one highest.

I also added a link to this documentation to README, since it had just a few examples and this provides a full explanation of what's supported

DroidBV8 commented 2 years ago

@oplik0

-- I have test the last release and I have this again:

image

-- For the command /roll display on topics it's Ok, I have seen her but not the result. Example:

image

For have the result displayed, I'm obliged to reload the page

image

Otherwise thank you for the link

oplik0 commented 2 years ago

Are you sure you have v2.1.3? NodeBB can sometimes be slow to update dependency versions, my instance still shows v2.1.1 as the latest. If you don't want to wait until it finally updates, you may need to install it manually (npm install nodebb-plugin-ws-dice@2.1.3 in NodeBB folder).

As for the refresh being required, I forgot my kind of janky code to force an update depended on what I was doing to hide the roll XD I think I'll try just doing it properly now though.

DroidBV8 commented 2 years ago

@oplik0

I was on 2.1.1, I'm on 2.1.3 now ;)

The bug of ">" is now fixed. Very good Job !!

The problem of refresh for result display on topics is still here like you said.

oplik0 commented 2 years ago

v2.2.0 should properly add the events without a reload now. And now it's implemented by actually calling posts.addTopicEvents after a post is loaded instead of refreshing part of the page :V

DroidBV8 commented 2 years ago

@oplik0

Wowwwwww, just testting 2.2.0 and it's very, very good ^^

I have just seen this, the time-badge and the text is not center like other version of the plugins. maybe this is intended?

2.2.0 : image

Before 2.2.0 :

image

Sorry to be so picky, if this bothers you let me know

EDIT:

You can simply fix it with :

.topic .posts.timeline .timeline-event:not(:first-child), .topic .posts.timeline>[component=post]:not(:first-child) {
    padding-top: 10px;
    padding-bottom: 10px;
}
oplik0 commented 2 years ago

Not sure how v2.2.0 could've affected this. Nevertheless, I tried a hopefully decent fix by using flex. The issue is making this work across themes, so I hope this will work more universally.

The issue is that it required changes to event text, so it'll only work with new rolls - at least until :has is enabled by default in Chrome/Firefox (it's behind a flag currently). It should work with earlier rolls in Safari though (CSS is somehow the one thing Safari is ahead in).

Edit: I think I'll also just try adding a migration script to solve this, instead of waiting for browsers to add a feature that is more then 15 years in the making

DroidBV8 commented 2 years ago

LOL @oplik0 :)

I have add CSS on NodeBB ACP for Waiting Fix ;)

DroidBV8 commented 2 years ago

@oplik0

now on 2.2.3

I don't know if it's better for dice icon align CSS but padding is OK !

image

I Fix it with CSS:

.df-event-icon {
    font-size: 3rem;
    display: inline-flex;
   position: relative;
   top: 4px !important;

}

image

oplik0 commented 2 years ago

Hmm... Not sure who it'd happen for you in the flexbox, are you sure it's a new roll? In Persona it works fine:

obraz

(d4 seems to be a bit higher than others because the icons are aligned by the numbers IIRC, not necessarily visually)

Anyway, I published v2.3.0 before reading this, with an automatically ran migration that will correct previous rolls on first run (it's overcomplicated since I wanted to create an easy to reuse module for migrations for my other plugins while I was at it :V), so if there is an issue with the new display on another theme it'd be a good idea to try to fix it here. Could you give me a link to your forum so I can see what's up with that? (here or via email if you don't want it to be public, I think I left it public :V)

DroidBV8 commented 2 years ago

Sorry for my late answer, I'm on holiday :) I tell you this on Saturday !!!

DroidBV8 commented 2 years ago

Hello @oplik0

I have test today your last version. I see this when the result is on 2 lines (Example: lot of dice roll)

image

How can we improve this?

Thanks

oplik0 commented 2 years ago

Due to the way topic events are shown (at least in Persona and probably most themes) I can't really move the username part anywhere. So thinking about this now probably the best solution would be to simply remove "for" from the text and pretend it's intentional.

DroidBV8 commented 2 years ago

yes it's a good idea @oplik0

oplik0 commented 2 years ago

Released v2.3.2 with that change in english and polish, but I see that you are using a custom french translation, so that's on you to fix (though if you wish to contribute it I'll gladly add it to the plugin :)

DroidBV8 commented 2 years ago

No problem @oplik0

I can contribute to your project ;) So, I provide you my traduction in french for the dice.json file here 👍

{
    "roll-one-die-0": "​A obtenu un",
    "roll-one-die-1": "sur un lancé de",
    "roll-one-die-2": "par",
    "roll-many-dice-0": "​A obtenu",
    "roll-many-dice-1": "(Lancé individuel:",
    "roll-many-dice-2": ") sur un lancé de",
    "roll-many-dice-3": "par",
    "too-many-nested-groups": "Trop de groupes de dés imbriqués, veuillez vous limiter à 10"
}
DroidBV8 commented 2 years ago

However, I still have the same problem with 2.3.2 ;) (Install manually, nodebb doesn't propose me 2.3.2)

Maybe delete user and span time of the result ?

I will test tomorrow, I'm tired.

Good night @oplik0 ;)

oplik0 commented 2 years ago

Again, the format is actually coming from the theme rather than this plugin. Basically, outside of switching to topic events, I'm largely powerless to change that. A theme could theoretically inline the topic events, but other things are using them (pins for example).

One could try to override the style for just the topic event containing dice rolls, but the issue here is that it's not simple to change style based on child elements (without :has at least). Ultimately, I'm mostly fine with how it currently looks. It's not perfect for long rolls, but at least from my experience these aren't that common, so I'm okay with them looking slightly janky tbh. At least I don't feel it's worth the effort fixing it would require from me.

DroidBV8 commented 2 years ago

Ok @oplik0

I'm on persona theme. I'm trying to fix the problem with CSS but it's difficult

DroidBV8 commented 2 years ago

EDIT: I see for : the fix: don't include the for in text is just on PL language. I'm wrong ?

DroidBV8 commented 2 years ago

RE @oplik0

I see this bug on chat on v2.3.4 :

image

very odd. it's not all the time


For the layout issue on the Persona theme in the timeline (I have test without any custom CSS), perhaps the best way would be to simply delete the "by Avatar USER X minutes ago of the result :

image


here is a fix traduction for 2.3.2 (I'm oblige to add a space before for "par" - = " par" for this version ) have a good day :)

{
    "roll-one-die-0": "​A obtenu un",
    "roll-one-die-1": "sur un lancé de",
    "roll-one-die-2": " par",
    "roll-many-dice-0": "​A obtenu",
    "roll-many-dice-1": "(Lancé individuel:",
    "roll-many-dice-2": ") sur un lancé de",
    "roll-many-dice-3": " par",
    "too-many-nested-groups": "Trop de groupes de dés imbriqués, veuillez vous limiter à 10"
}
oplik0 commented 2 years ago

EDIT: I see for : the fix: don't include the for in text is just on PL language. I'm wrong ?

No, English (look under en-GB) was also changed. However, as I noted, since you are using your own translation I can't change it.

Also, again, the username and timestamp are actually part of the topic event and the plugin doesn't control them. I probably could build some over-engineered CSS solution or, alternatively, stop using topic events and make a replacement myself, but frankly, both are too much work for me for such a minor issue.

That is: I'd accept a PR with some fix, but won't spend more of my time working on it.

DroidBV8 commented 2 years ago

Ok I understand @oplik0

oplik0 commented 2 years ago

Btw. would you be fine with me adding your French translation to the repository? Maybe it'll help someone else, and if I make some slight change to formatting again I'll try to modify that translation too

DroidBV8 commented 2 years ago

@oplik0

no problem to be a contributor on your project.

DroidBV8 commented 2 years ago

hello @oplik0

I asked to the NodeBB dev and my friend phenomlab of Sudonix why and here is his answer.

https://community.nodebb.org/post/88888

EDIT: https://github.com/NodeBB/NodeBB/blob/master/public/src/modules/helpers.common.js#L199-L221 https://sudonix.com/post/3974

DroidBV8 commented 2 years ago

Hello @oplik0

Wouhhooooooo NodeBB V 2.5.0 is here :clap:

Can not wait to see the result !!!

https://community.nodebb.org/post/88900

DroidBV8 commented 2 years ago

Hello @oplik0

How are you ?

I installed the plugin in production but it looks like there are still a few issues :(

Examples: image

The /roll command is incorrectly transcribed in the result in certain cases apparently.

Apparently the = is a problem but there may be others

Possible to fix that ?

Thanks ;)

oplik0 commented 2 years ago

The first case is just scientific notation - that's what you get of you roll too much. That's actually quite a bit beyond a safe integer in JS, so the result can't even be accurate (this is around 2^136. Maximum safe integer is 2^53-1).

I'd say it's the correct way to show it instead of a 42-digit number of which most would be zeroes due to lost precision.

As for the second, I'll fix it and look into what other useful characters might be escaped when they shouldn't.

DroidBV8 commented 2 years ago

Hello @oplik0

Thanks four your answer :)

I noticed for the first case and wait for the second,

Keep the good work.

Thank you

oplik0 commented 1 year ago

Done, there should be no more escaping problems now :) (in 2.4.1)

And to expand on the first case: it might just be slightly unituitive that that notation means 9 to the power of the sum of 10 d7 rolls - which in this case is 9^43, which is a lot :)

DroidBV8 commented 1 year ago

Hello @oplik0

I'm on V.2.4.2 and for me the problem is stil here with FR language (not test with english)

image

A screen :

image

And for the first case, yes it's a very very big roll ha ha ;)

oplik0 commented 1 year ago

Ah, I forgot to test it on chat... The issue again is that the message on chat is being escaped beyond my control. My solution for 2.4.3 is to turn the escaped characters into their fullwidth versions - it doesn't look ideal (weird spacing) but at least you can see what the roll was: obraz

DroidBV8 commented 1 year ago

Yes it's better @oplik0 ;) I will test the last version tomorrow because i'm tired now.

Thanks for your hardwork dude ;)

DroidBV8 commented 1 year ago

hello @oplik0

It seems that 2.4.3 version have a big problem.

-->When she's is activate , we have not the possibilities to edit post on the forum. We can choose edit button, change the text in WYSIWYG but not work after...

We downgrade in 2.4.2 because we have not the problem of edit post with it

DroidBV8 commented 1 year ago

Hello @oplik0

just for my information, have you seen the message above ? I have test on a fresh install VM and it's the same result :(

cya

oplik0 commented 1 year ago

Yes, just had other things to do today, I'll look into this tomorrow.

oplik0 commented 1 year ago

@DroidBV8 v2.5.0 should resolve this, and while I was at it, I improved the way rolls are hidden (now it will still show rolls if you have multiple of them in a post but no other text and hide them all if there is any other text)

DroidBV8 commented 1 year ago

Thanks @oplik0

I will test this versions as soon as possible ;)

DroidBV8 commented 1 year ago

Hi @oplik0

Great, 2.5.0 works now without the bug mentioned earlier. (editing posts and in the Chat)

I share with you this seen in 2.50:

--> I note that now the dice are no longer displayed much (very very little) in the rolls in the topics in favor of the simple display of the results in figures:

image

Is this normal or it's a bug? If so, it's a shame (dommage) because I thought it was super nice to see the dice ;)

It's also a shame that they don't appear in the Chat results, but maybe it wouldn't be super readable.

What do you think ?

Looking forward to read your reply