Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

Add Markdown support. #2832

Merged
merged 14 commits into from
Mar 12, 2016
Merged

Add Markdown support. #2832

merged 14 commits into from
Mar 12, 2016

Conversation

anoadragon453
Copy link
Contributor

Underline, Italics, Strikethrough and Bold supported. Addresses #2497

example

@zetok
Copy link
Contributor

zetok commented Jan 21, 2016

In its current form it interferes with user input.

The proper way to do it was discussed in Tox-Archive/Tox-STS#74 - in short, new message type is needed in toxcore, and using that there can be a proper markdown support done.

@anoadragon453
Copy link
Contributor Author

Hmm, I see. So there needs to be some sort of differentiation between markdown and non-markdown text.

Which we could technically add with something similar to Wikipedia's "nowiki" tag, essentially cancelling out md detection within those brackets, but a user with a different client would see those brackets and be confused.

Interesting. Well, when we do get a separate message type, this code'll be here to parse it :)

@Zer0-One
Copy link
Contributor

@anoadragon453 You might be interested in catching up on the discussion here: Tox/Tox-Client-Standard#25

@anoadragon453
Copy link
Contributor Author

Thank you for the link, interesting read. A standard spec is definitely required if we are to go through with it.

In my personal experience, some users turn down Tox specifically because it doesn't feature basic text formatting.

IMO Toxcore should have a separate message type, each client should have a standard, non-instrusive way to switch between the two, and clients that can't support certain features should support as much as possible, and no more.

Hopefully we figure out what definite route we're taking soon so we can get some implementation going.

else if (exp.cap(7) == "_" && snippet.length() > 2) // Italics _text_
htmledSnippet = QString("<i>%1</i>").arg(snippet.mid(1,snippet.length()-2));
else if (exp.cap(10) == "__"&& snippet.length() > 4) // Italics __text__
htmledSnippet = QString("<i>%1</i>").arg(snippet.mid(2,snippet.length()-4));
Copy link
Contributor

Choose a reason for hiding this comment

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

Italics 3 times but bold once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agilob What other pattern for bold should be added? :P

It'll have to be adapted to the STS' spec eventually anyways. It's pretty simply to edit/add new syntaxes though.

On January 21, 2016 5:04:32 AM PST, agilob notifications@github.com wrote:

  •    int offset = 0;
    
  •    while ((offset = exp.indexIn(out, offset)) != -1)
    
  •    {
    
  •        QString snippet = exp.cap();
    
  •        QString htmledSnippet;
    
  •        // Match captured string to corresponding md format
    
  •        if (exp.cap(1) == "**") // Bold **text**
    
  •            htmledSnippet =
    
    QString("%1").arg(snippet.mid(2,snippet.length()-4));
  •        else if (exp.cap(4) == "_" && snippet.length() > 2) //
    
    Italics *text_
  •            htmledSnippet =
    
    QString("%1").arg(snippet.mid(1,snippet.length()-2));
  •        else if (exp.cap(7) == "_" && snippet.length() > 2) //
    
    Italics text
  •            htmledSnippet =
    
    QString("%1").arg(snippet.mid(1,snippet.length()-2));
  •        else if (exp.cap(10) == "**"&& snippet.length() > 4) //
    
    Italics __text**
  •            htmledSnippet =
    
    QString("%1").arg(snippet.mid(2,snippet.length()-4));

Italics 3 times but bold once?


Reply to this email directly or view it on GitHub:
https://github.com/tux3/qTox/pull/2832/files#r50397534

Copy link
Contributor

Choose a reason for hiding this comment

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

Your code:

** - bold
* - italics
_ - italics
__ - italics

** - should make bold otherwise is mega inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

** - should make bold otherwise is mega inconsistent

Err, __.
According to github editor, __ results in bold.

@zetok
Copy link
Contributor

zetok commented Jan 21, 2016

@anoadragon453 irungentoo/toxcore#1502 here you go, you can checkout branch and make needed stuff on qTox side :)

@anoadragon453
Copy link
Contributor Author

Thanks, I'll start on the framework for differentiating between different message types.

Edit: Nevermind, already implemented. Just need to add a new type.

Edit 2: Of course we use a bool to determine message type...

Edit 3: Perhaps we should put a new message type on hold and consider other options... see Tox/Tox-Client-Standard#25 (comment)

@zetok
Copy link
Contributor

zetok commented Jan 23, 2016

Edit 3: Perhaps we should put a new message type on hold and consider other options...

Well, you're the one implementing it in qTox, so it's up to you when you decide to work on it. :)

The way I see it (perhaps biased a bit) is that it's going to be done via MARKDOWN message type.

Also, I don't see other options..?

@ProMcTagonist
Copy link
Contributor

@anoadragon453
Copy link
Contributor Author

I think implementing it in toxcore is fine, but we have to do two things to implement it:

  • Be aware that MD messages may potentially break clients that have not been updated to work with them yet.
  • Figure out a standardized way for the client to determine that the user wants to send a normal or MD message - be it either a button toggle or something within the message itself (think '/me', /md perhaps?).

Once those two things are figured out and agreed upon, I'll be more than happy to crank out the code to make it work.

@ProMcTagonist
Copy link
Contributor

needing to manually switch to markdown mode is stupid and a pain

the point of markdown is being readable whether it's styled or not and being integrated into plaintext

@anoadragon453
Copy link
Contributor Author

@ProMcTagonist Good point, if we're not doing another message type, then there's no need to check.

@tux3
Copy link
Member

tux3 commented Jan 23, 2016

@ProMcTagonist
I 100% agree, I think it's not natural to have to switch modes when you just want to chat.
And if people really feel it's important to not remove anything from the message another option is to render the markdown while keeping the markings, like GMANE does:

http://thread.gmane.org/gmane.text.docutils.user/4104/focus=4105
http://thread.gmane.org/gmane.mail.mutt.user/43722/focus=43750

But in my opinion this is not a problem.

@zetok
Copy link
Contributor

zetok commented Jan 23, 2016

the point of markdown is being readable whether it's styled or not and being integrated into plaintext

That doesn't matter. What matters, is whether you can send a non-marked by markdown message.

@tux3 how do you plan on doing this, when there'll be only md-marked messages?

@ProMcTagonist
Copy link
Contributor

People sending codeblocks through tox should use the MD code formatting because otherwise it'll get all stripped in most clients anyway so codeblocks are an argument pro-constant-markdown

@tux3 That looks like a neat solution, no characters are removed -> no problem

@anoadragon453
Copy link
Contributor Author

@zetok
Copy link
Contributor

zetok commented Jan 23, 2016

@anoadragon453 No.

@tux3
Copy link
Member

tux3 commented Jan 23, 2016

@zetok
I don't see the case for sending non-markdown messages, though.
If you simply don't like Markdown, we will probably offer an option to not render it.
But do you need a special messages that forces other people to not use markdown?

@TheNain38
Copy link
Contributor

Yes, it will be best to have an option to not render it if they don't like it. @tux3 +1

@ProMcTagonist
Copy link
Contributor

I vote for constant markdown parsing, in the normal message type, keeping signal characters like gmane, and an option not to render styling

@TheNain38
Copy link
Contributor

The problem with not removing signal characters is that you have the obligation to have them visible to have the styling.
And in the case of Markdown signal characters is the "replacement" to non rendered styiling

@zetok
Copy link
Contributor

zetok commented Jan 23, 2016

@tux3

I don't see the case for sending non-markdown messages, though.
If you simply don't like Markdown, we will probably offer an option to not render it.

It's not about liking it or not - it's about preserving information, with which markdown interferes.

Depending on the degree of "support" for markdown, it wouldn't interfere much, or would interfere to a degree where receiving (& rendering) side would have some info from message simply stripped.

Option to not render it (some quick switch [under r-click?]) is probably the solution. :)

@ProMcTagonist
Copy link
Contributor

@zetok

It's not about liking it or not - it's about preserving information, with which markdown interferes.

This was my concern too, until:

@tux3

And if people really feel it's important to not remove anything from the message another option is to render the markdown while keeping the markings, like GMANE does:
http://thread.gmane.org/gmane.text.docutils.user/4104/focus=4105
http://thread.gmane.org/gmane.mail.mutt.user/43722/focus=43750

So everything is preserved.

@tux3
Copy link
Member

tux3 commented Jan 23, 2016

@zetok
We will have an option to not render Markdown then.
Now if markdown is enabled, should we do it the GMANE way or strip signaling characters?
Personally I'm okay with the GMANE option, although I don't think it's really needed.

@anoadragon453
Copy link
Contributor Author

I'd like to point out that copying a MD'd message perseveres the non-parsed text.

@ProMcTagonist
Copy link
Contributor

@anoadragon453 it should be viewable without pasting to a text editor, which gmane solves nicely

@anoadragon453
Copy link
Contributor Author

@ProMcTagonist I'm unfamiliar with Gmane, how does it do this exactly?

@TheNain38
Copy link
Contributor

I'd like to point out that copying a MD'd message perseveres the non-parsed text.

+1
Also the problem in keeping signal characters is that the non-parsed text is the "replacement" to parsed text with Markdown...

@@ -54,6 +56,10 @@ ChatMessage::Ptr ChatMessage::createChatMessage(const QString &sender, const QSt
//quotes (green text)
text = detectQuotes(detectAnchors(text), type);

//markdown
if (Settings::getInstance().getMarkdownPreference() != 0)
Copy link
Member

Choose a reason for hiding this comment

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

You should use an enum for the preference instead of a magic value

@sudden6
Copy link
Member

sudden6 commented Jan 31, 2016

@anoadragon453 could you please write a few words about this new feature in the user manual?

@anoadragon453
Copy link
Contributor Author

@sudden6 Sure, just in the wiki?

@sudden6
Copy link
Member

sudden6 commented Jan 31, 2016

The user facing things are documented in /doc/user_manual_en.md

@anoadragon453
Copy link
Contributor Author

@sudden6 Added, thanks.

@anoadragon453
Copy link
Contributor Author

@tux3 Addressed the issues.

@tux3
Copy link
Member

tux3 commented Feb 1, 2016

I'll take a look at it as soon as I can

@anoadragon453
Copy link
Contributor Author

@zetok Good catch, fixed.

@anoadragon453
Copy link
Contributor Author

@tux3 Anything update on this?

@anoadragon453
Copy link
Contributor Author

⌛ ...

@quininer
Copy link
Contributor

⌛ ... +1

@Deleetdk
Copy link

Any update on this? I downloaded the most recent version and I don't see any markdown options. Presumably, this branch wasn't merged yet?

@ProMcTagonist
Copy link
Contributor

@Deleetdk This PR has not yet been merged. You can see this indicator next to the title in Github's web interface that lets us know it's still open:

open

When it turns purple and says "Merged," you'll know a PR was merged. (Red with "Closed" means it's rejected.)

@anoadragon453
Copy link
Contributor Author

Are we still waiting for the Tox Standard to decide the md rules?

On March 11, 2016 1:15:06 PM PST, ProMcTagonist notifications@github.com wrote:

@Deleetdk This PR has not yet been merged. You can see this indicator
next to the title in Github's web interface that lets us know it's
still open:

open

When it turns purple and says "Merged," you'll know a PR was merged.
(Red with "Closed" means it's rejected.)


Reply to this email directly or view it on GitHub:
#2832 (comment)

@tux3
Copy link
Member

tux3 commented Mar 12, 2016

Due to the inability of the Tox Standard to actually standardize things, we will move forward with Markdown support now and deal with standard conformance when there is something to conform to :)

@tux3 tux3 merged commit e04bd91 into qTox:master Mar 12, 2016
@Deleetdk
Copy link

How long does it take for the builds to be updated? Specifically, the Windows 64-bit one.

@tux3
Copy link
Member

tux3 commented Mar 12, 2016

For the nightly builds, usually around ~30 minutes but since Jenkins is really busy at the moment count an hour or two.

@zetok zetok mentioned this pull request Mar 12, 2016
@anoadragon453
Copy link
Contributor Author

Thanks for merging, I'll keep an eye on how the standard progresses and update accordingly 👍

@zetok
Copy link
Contributor

zetok commented Mar 12, 2016

Thanks for merging, I'll keep an eye on how the standard progresses and update accordingly 👍

"standard" is IMO currently a ~joke, and thus waiting for it is most likely a waste of time.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants