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

[Feature]: Add proper html support for token notes. #3791

Closed
thelsing opened this issue Dec 26, 2022 · 13 comments · Fixed by #3794 or #3879
Closed

[Feature]: Add proper html support for token notes. #3791

thelsing opened this issue Dec 26, 2022 · 13 comments · Fixed by #3794 or #3879
Assignees
Labels
feature Adding functionality that adds value tested This issue has been QA tested by someone other than the developer.

Comments

@thelsing
Copy link
Collaborator

Feature Request

Currently one can use html tags in token notes, but there is no proper html editor for them. Also the hover display inserts
for every linebreak in the notes. One can only work around it by having all html code in one line.

The Solution you'd like

I would also like to be able to just copy & paste content from websites into those fields and keep formating.
Best would be a WYSIWYG editor for html with the ability to edit the source code if necessary.

Alternatives that you've considered.

Only using one line of html text. But I still can't paste HTML this way.

Additional Context

No response

@thelsing thelsing added the feature Adding functionality that adds value label Dec 26, 2022
@thelsing thelsing self-assigned this Dec 26, 2022
@thelsing
Copy link
Collaborator Author

I already have this in my personal use branch. I combined the JFX HtmlEditor control with rsyntaxtextarea.

@Phergus
Copy link
Contributor

Phergus commented Dec 27, 2022

I combined the JFX HtmlEditor control with rsyntaxtextarea

Any potential issues with that? Likely to break on future Java releases?

@thelsing
Copy link
Collaborator Author

Not more likely than our other JavaFX Controls breaking. Its just a JPanel with two tabs. Nothing complicated:
image
image

Only issue is that JavaFX controls don't use the flatlaf themes.

@thelsing
Copy link
Collaborator Author

I will wait with a PR until #3785 is merged because this change depends on that.

@Phergus
Copy link
Contributor

Phergus commented Dec 27, 2022

Nice.

@Phergus Phergus moved this from Todo to In Progress in MapTool 1.13.0 Dec 27, 2022
@github-project-automation github-project-automation bot moved this from In Progress to Merged in MapTool 1.13.0 Dec 27, 2022
@Phergus Phergus moved this from Merged to Needs Testing in MapTool 1.13.0 Dec 27, 2022
@Phergus
Copy link
Contributor

Phergus commented Feb 20, 2023

Tested. Better HMTL support in notes. Seems to add in extra whitespace but the HTML can be edited directly if needed.

@Phergus Phergus moved this from Needs Testing to Done in MapTool 1.13.0 Feb 20, 2023
@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label Feb 20, 2023
@kwvanderlinde
Copy link
Collaborator

I like this editor, it looks handy.

I'm going to push back on one aspect of these changes though: we should not force notes to be HTML. There are existing campaigns and well-established frameworks that use notes for purposes other than the built-in notes overlay, and which cannot function properly if the notes are modified. E.g., I personally make use of Rod's 5e framework which makes liberal use of markdown, including storing it in notes / GM notes. With the notes being turned into HTML documents, the framework is no longer able to properly render the markdown into HTML fragments that can be inserted into frames, and it breaks the custom roll links built into the framework.


And one minor bug I found while testing that out: most stamps will now count as markers since even empty notes and GM notes still store HTML content. So clicking on an object with no notes, GM notes or portrait results in a teeny tiny empty notes overlay:
image

Brand new tokens are exempt until edited.

@Azhrei
Copy link
Member

Azhrei commented Mar 17, 2023

I'm going to push back on one aspect of these changes though: we should not force notes to be HTML.

Agreed.

Do we have control over the tabs at the bottom? Could a third one for Markdown be added? IIRC, rsyntaxarea is capable of supporting just about any syntax highlighting if we can find/create our own regexes to handle where each type of highlighting begins and ends.

For upward compatibility... When loading a campaign, if the first characters (skipping leading whitespace, of course) are <html then we can treat the field as HTML. If any of the first 5-10 lines start with # then treat it as Markdown. Anything else is plain text. Might need two new fields in Preferences: Fallback text type (Markdown / Plain text) in case Markdown should be the default when the type cannot be determined, and Override text type with fallback value (yes/no) to force the fallback type to always be used for non-HTML notes. (Ugh. Better ideas are welcome.)

@thelsing
Copy link
Collaborator Author

Maybe we should just put the format of the notes into the campaign properties? Not sure if it different token in the same campaign should have different formats of notes.
If this should be possible then we should probably add the noteformat to the token.

@thelsing
Copy link
Collaborator Author

For upward compatibility... When loading a campaign, if the first characters (skipping leading whitespace, of course) are <html then we can treat the field as HTML. If any of the first 5-10 lines start with # then treat it as Markdown. Anything else is plain text. Might need two new fields in Preferences: Fallback text type (Markdown / Plain text) in case Markdown should be the default when the type cannot be determined, and Override text type with fallback value (yes/no) to force the fallback type to always be used for non-HTML notes. (Ugh. Better ideas are welcome.)

IMO we should just add relevant fields to Token or CampaignProperties and check if we are opening an old campaign and ask the use what type of notes he wants.
Autodetection will cause more harm than good. Who knows what the different frameworks store in the notes. JSON? base64 encoded images?

@Azhrei
Copy link
Member

Azhrei commented Mar 17, 2023

Auto detection would always fallback to plain text (unless the user selects otherwise in Preferences). So only HTML and Markdown formats would be processed (HTML AS IS, while Markdown would be converted to HTML on the fly). We already auto detect a lot of stuff when reading the campaign and always have; it’s one reason why MT can read campaigns from 15 years ago with minimal impact.

Each token has its own settings, as some on the Object layer may just be informational and used by the GM or players as reference points, while others might have more formatted info in them or even data (as you point out).

@kwvanderlinde
Copy link
Collaborator

Maybe we should just put the format of the notes into the campaign properties? Not sure if it different token in the same campaign should have different formats of notes.
If this should be possible then we should probably add the noteformat to the token.

From my own experience, it is sometimes useful to have different tokens use different formats. E.g., most tokens operate under a framework, but there are other that you do want to use with the built-in notes display. Or by using more than one framework, each with different assumptions.


I think we should just aim to have MT be less opinionated about the contents of the notes. This can be done with small tweaks to the current state of things. We also don't need special support for markdown or any other kind data, as long as we leave things alone unless asked to do otherwise (we could have support for others, but it's in no way needed).

Here is an example of how we can preserve existing data while keeping things mostly the same:

  • The first thing is to not have the Token getters convert to HTML.
  • After that, I noticed the Text tab does not convert the contents to HTML, while (of course) the HTML tab does. So we can prefer showing the Text tab rather than the HTML tab, and that would leave the data alone. The highlighting alone is a nice enhancement for those who do want to edit HTML by hand. And it's also not a bad deal for anyone who write markdown by hand, since HTML tags can be embedded in markdown too.
  • If desired, the user can switch to the WYSIWYG editor, which will convert to an HTML document now that it's been requested.

And there's plenty of room to do other things on top of that that could improve the user experience. E.g.:

  • If the user switches to the HTML tab, persist that choice in the Token so they don't have to switch tabs for it in the future.
  • If the user mistakenly switches to the HTML, makes no edits, and then goes back to the TEXT tab, we can have the original contents waiting for them. No harm no foul 🙂
  • Add a campaign property for users to say "I really do want all my token notes to be HTML no matter what".
  • Add a syntax highlighting selector similar to that in the token property editor. Optionally try to guess what highlighting to use.

I'm not actually suggesting or requesting any of these, I only put them there to prove the point.


Shifting gears, I was doing more testing and found another quirk, which I guess is what Phergus is referring to as well:

Seems to add in extra whitespace but the HTML can be edited directly if needed.

There can be quite a bit of extra vertical space added depending on the size of the notes. E.g.,:
image

This seems to be the result of embedded a complete HTML document (<html> and all) inside of a <table>. If I instead hack in the contents that would be inside the <body>, it looks more natural, and is also in line with previous behaviour:

image

From this angle, it makes sense to extract the (<body>) contents of HTML notes for inclusion in the notes overlay rather than injecting the entire document. Though I say this without considering the ramifications of eliminating the <head> or other potential pitfalls.

@thelsing thelsing reopened this Mar 21, 2023
@github-project-automation github-project-automation bot moved this from Done to In Progress in MapTool 1.13.0 Mar 21, 2023
@thelsing
Copy link
Collaborator Author

I'll add a text type for notes and gmnotes for now with text/plain as default. Supported mime type will be text/plain, text/html and text/markdown. I'll reorder the tabs making TXT the first one the HTML tab also works as a (not editable) preview for markdown.

I'll also only store the content of the body tag from the html editor.

@github-project-automation github-project-automation bot moved this from In Progress to Merged in MapTool 1.13.0 Mar 28, 2023
@kwvanderlinde kwvanderlinde moved this from Merged to Done in MapTool 1.13.0 Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding functionality that adds value tested This issue has been QA tested by someone other than the developer.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants