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

Replace Floofchat with ArmoredChat #961

Merged
merged 33 commits into from
Jul 11, 2024

Conversation

Armored-Dragon
Copy link
Member

@Armored-Dragon Armored-Dragon commented May 17, 2024

Replace Floofchat with a smaller application using written in QML.

image
image

Compatibility

This version of Armored Chat is not compatible with Floofchat.
Armored Chat is still compatible with notificationCore.js.

Linked issues and pulls

Merge with overte-org/community-apps#79.
Part of #886.
Closes #898 as complete.
Closes #778 as irrelevant.
Closes #922 as irrelevant.
Closes #975 as irrelevant.

Todo:

  • Embedding
    • Handle link embeds
    • (Potentially) handle special embeds like YouTube and GitHub
  • Selective auto-scroll
    • Prevent scrolling to newest chat message if the view is not near the bottom.
  • Settings
    • Clear history
    • Set maximum amount of saved messages
    • Toggle external window
  • Docs
  • Quick-Message bar (related Floofchat quick chat has sticky keys #975)

Floof Chat moved to script archive.
Fixed local message distance check.

Signed-off-by: Armored Dragon <[email protected]>
Erase messages.
Toggle external window.

Signed-off-by: Armored Dragon <[email protected]>
Signed-off-by: Armored Dragon <[email protected]>
@Armored-Dragon Armored-Dragon self-assigned this May 21, 2024
Settings now initialize properly.
Settings now save properly.
 Removed compact_chat setting as deprecated.
 Added spacing between settings.
 Message history now prunes itself if the number of messages exceeds maximum.

Signed-off-by: Armored Dragon <[email protected]>
Hotfix for empty message_history setting.

Signed-off-by: Armored Dragon <[email protected]>
@Armored-Dragon
Copy link
Member Author

Interesting note: When testing this application I ran into seemingly random variations in the way dates are displayed. The intended way for dates to be displayed, as an example, is [ HH:MM:SS - Month Day, Year] or for today [ 13:10:25 - May 22, 2024]. This is the outcome of the date when executing the application though Steam. I use Steam as a launcher by adding the .appimage as a non Steam game.
image

After you submit a new message, the date will display as a different format.
image

When executing the very same executable in a terminal, the dates come out differently. This is the result of sending a new message.
image

I am not certian on the cause of this yet, but the code for creating the dates are idendical.
If nothing else, please keep this in mind when reviewing this pull request.

Messages are now rich text.
Fix clear_message command flow over.

Signed-off-by: Armored Dragon <[email protected]>
Hopefully makes code pasting easier.
Added missing break to prevent unintended executions.

Signed-off-by: Armored Dragon <[email protected]>
Allows for kinetic mouse scrolling.

Signed-off-by: Armored Dragon <[email protected]>
Signed-off-by: Armored Dragon <[email protected]>
Disabled sending on Shift + Enter to prevent confusion.

Signed-off-by: Armored Dragon <[email protected]>
@Armored-Dragon
Copy link
Member Author

Armored-Dragon commented May 22, 2024

Despite my best efforts I have not found a reliable way to impliment "Selective auto-scroll".

For reviewers:

@Armored-Dragon Armored-Dragon marked this pull request as ready for review May 22, 2024 10:08
@Armored-Dragon

This comment was marked as resolved.

@Armored-Dragon Armored-Dragon marked this pull request as draft May 22, 2024 21:00
Signed-off-by: Armored Dragon <[email protected]>
Signed-off-by: Armored Dragon <[email protected]>
@Armored-Dragon Armored-Dragon marked this pull request as ready for review May 23, 2024 01:56
@Armored-Dragon Armored-Dragon added needs QA This pull request needs to be tested needs CR This pull request needs to be code reviewed labels May 23, 2024
Resolved "listview" rendering warnings.
Prevent sending empty messages.

Signed-off-by: Armored Dragon <[email protected]>
Signed-off-by: Armored Dragon <[email protected]>
Signed-off-by: Armored Dragon <[email protected]>
Document image embedding.
Declared notificationCore as dependency.

Signed-off-by: Armored Dragon <[email protected]>
Updated systemchat html script path.
Renamed "chat" to "floofChat" to avoid confusion.

Signed-off-by: Armored Dragon <[email protected]>
More documentation on link related features.

Signed-off-by: Armored Dragon <[email protected]>
@Armored-Dragon
Copy link
Member Author

Armored-Dragon commented Jun 7, 2024

Here is a list of tasks to do gathered from QA testing:
Store messages in private settings to prevent eavesdropping.
- #1009

  • VR Keyboard does not show up.
  • Scrolling to the bottom goes too far sometimes.

@ksuprynowicz
Copy link
Member

VR keyboard does not show up

Signed-off-by: Armored Dragon <[email protected]>
Signed-off-by: Armored Dragon <[email protected]>
@ksuprynowicz
Copy link
Member

It also looks like the messages from Floof Chat are not being received. We could drop Floof Chat compatibility later, but having no compatibility in initial version will cause problems since some people will be running stable version and some will be running development builds.
Version of Armored Chat from Community Apps is compatible with Floof Chat.

Made conversion functions to allow communication between apps.
Removed developer console.log function.

Signed-off-by: Armored Dragon <[email protected]>
@Armored-Dragon
Copy link
Member Author

I've restored compatibility with floofchat in 192d80a, I'm hoping the the future we can toss the compatibility. Maybe after the next release?

@ksuprynowicz
Copy link
Member

Maybe after the next release?

Sure! That's a very good idea

@Armored-Dragon

This comment was marked as outdated.

@ksuprynowicz
Copy link
Member

I will try to look at VR keyboard issue this weekend

Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost everything looks good, although there's one thing that we should test thoroughly to make sure it's not a security vulnerability that could allow arbitrary JS code execution with QML priviledges.

Removed now redundant <script> filtering.

Signed-off-by: Armored Dragon <[email protected]>
Signed-off-by: Armored Dragon <[email protected]>
@ksuprynowicz
Copy link
Member

I found such exception somehow just now, but I have trouble repeating it:

[07/09 20:11:59] [WARNING] [overte.scriptengine] run ---------- UNCAUGHT EXCEPTION --------
[07/09 20:11:59] [WARNING] [overte.scriptengine] runInThread Exception: "Uncaught TypeError: Cannot read properties of undefined (reading 'toLowerCase')" "[Error in signal proxy]"  at line  102 , column  42 Backtrace: ("TypeError: Cannot read properties of undefined (reading 'toLowerCase')", "    at receivedMessage (file:///home/ksuprynowicz/overte/overte_v8/overte/cmake-build-debug/interface/scripts/communityScripts/armored-chat/armored_chat.js:102:43)")

@ksuprynowicz
Copy link
Member

I also noticed that messages from FloofChat don't show up.

@Armored-Dragon
Copy link
Member Author

I found such exception somehow just now, but I have trouble repeating it:

[07/09 20:11:59] [WARNING] [overte.scriptengine] run ---------- UNCAUGHT EXCEPTION --------
[07/09 20:11:59] [WARNING] [overte.scriptengine] runInThread Exception: "Uncaught TypeError: Cannot read properties of undefined (reading 'toLowerCase')" "[Error in signal proxy]"  at line  102 , column  42 Backtrace: ("TypeError: Cannot read properties of undefined (reading 'toLowerCase')", "    at receivedMessage (file:///home/ksuprynowicz/overte/overte_v8/overte/cmake-build-debug/interface/scripts/communityScripts/armored-chat/armored_chat.js:102:43)")

This seems to occur using the alternate chat application. Really any other chat application other than floofchat (also excluding this chat app of course) will cause this issue. I will put in a sanity check.
The issue is there is no "channel" key in the message object. It is trying to turn an undefined value into a lowercase string.

@ksuprynowicz ksuprynowicz added CR approved This pull request has been successfully code reviewed QA approved This pull request has been successfully tested and removed needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested labels Jul 11, 2024
@ksuprynowicz ksuprynowicz merged commit 1d1a680 into overte-org:master Jul 11, 2024
6 checks passed
@Armored-Dragon Armored-Dragon deleted the ArmoredChat branch October 30, 2024 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR approved This pull request has been successfully code reviewed QA approved This pull request has been successfully tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Floofchat quick chat has sticky keys Chat breaks URLs Pop out chat window
2 participants