Formatting and flicking for preformatted text

[blackbeam]

This is quite ugly but realy cheap solution.

Only comments with preformatted text become flickable. Downside is that now user will have to click on a header of a flickable comment to select it, because in other case click event will be stolen.

commentBodyTextInnerHidden required to properly set Flickable.contentWidth. I couldn't see another way to do it cheaply.

[accumulator]

Thanks for this patch, but I agree, it's really ugly :)

First of all, the addition to the parser code is incorrectly converting all spaces to html non-breaking spaces. The existing replace for \n is symmetric, the space replace is not.

Then, the check in QML model.body.indexOf("<pre>") is not cheap, and it's duplicated in multiple places, so there's definately a better way to do that, preferably setting a property in the parser code, where it's checked already. Also the model.body.replace is very expensive, and I don't immediately see what you're trying to accomplish there.

It might be possible to detect horizontal overflow by checking the content width of the commentBodyText against its parent Column?

[blackbeam]

Also after a few days of usage i realized that i totally forgot to implement same for a text of a post :) Anyway, thanks for review. I will walk through your comments in my spare time.

[blackbeam]

Could you please review this again.

1. Spaces now preserved only within <pre> blocks.
2. Not contentWidth but paintedWidth was very helpful and now there is no hidden QML elements so thanks for the hint.
3. Also there is no model.body.indexOf("<pre>") anymore because of 2.

[accumulator]

Regarding (1): I think you misunderstood my previous comment. With symmetric I meant that some elements (like newlines) are replaced with something unique that survives html-unescape, and still be unique after unescaping, so it can be replaced back to its original. For newlines I used "<>" since the empty element is unique (since what we get from the server is html-escaped html, we can assume this).

If you replace a space with "&nbsp;" this is after unescaping still "&nbsp;", and therefore not unique anymore since it's indistinguishable from "&nbsp;" already occurring in the unescaped html. In your code, you also do not replace it back into spaces after unescaping, so you basically change the final html. In this case that's not really a problem though, since <pre> allows html entities and renders it correctly.

may I suggest to do it as follows (to keep the code simpler):

if (html.contains("<pre>")) {
    QString shieldedHtml(html);
    shieldedHtml.replace(" ","<s>");
    shieldedHtml.replace("\n", "<>");
    document.setHtml(shieldedHtml);
    unescaped = document.toPlainText();
    unescaped.replace("<>", "\n");
    unescaped.replace("<s>", " ");

[accumulator]

Regarding (2) and (3) : great!

With regard to the Flickable listitem now capturing click and drag events, I think this is a bit jarring to the user experience. Right now I see basically two ways to solve this in an acceptable manner:

1) enable the item flickable after detecting a horizontal drag on the item, so the click still works to show the menu. Problem is how to synthesize the pressed and/or drag-start events on the initially disabled Flickable.. 2) add a small icon in the top/bottom-right corner of the listitem that shows there's more text to the right, which enables the flickable when clicked.

[blackbeam]

Regarding (1) I think i found the reason of misunderstanding

If you replace a space with "&nbsp;" this is after unescaping still "&nbsp;"

Thats not true, because &nbsp; is HTML entity and toPlainText will replace it with space character and you do not need second call to replace.

I reimplemented space preserving only for <pre> because afraid that result may differ if not preformatted plain text will get more spaces. But i made some tests and realized that it is not the case because textFormat: Text.RichText do it's job.

Messing with user's  s is also not possible because they are   at that moment.

So initial implementation with only one replace shieldedHtml.replace(" "," "); was the shortest and i think fastest.

What do you think?

[accumulator]

You are right, a   gets replaced by a space, and escaped   are safe from matching. Any whitespace that is preserved if we replace all spaces outside <pre> is not a problem since it's html, which means it gets ignored.

[accumulator]

I just tested your last commit regarding the click and drag behaviour. It's a bit quirky still, but I like it. The click works as expected. The item highlight on hold is gone, and the horizontal drag only works from the second go, but I think that's minor.

If you can revert c89bea4 and 5cf93bf I'm fine pulling this in.

[accumulator]

Hmm it seems this last commit has some side effects. It looks like half (?) of the vertical flicks are ignored.

[blackbeam]

c89bea4 and 5cf93bf reverted.

propagateComposedEvents somehow behaving awfully, part of events just disappears. I replaced it with manual call to commentDelegate.clicked()

[accumulator]

Great, works fine now.

I'm afraid there's still one more issue :)

First I thought it had to do with my upgrade to SFOS2.0, but it seems to be caused by a change in this branch. Try for yourself to find a thread with a lot of comments, then retrieve a few extra comments using the buttons. You'll see that the newly added listitems are misaligned and overlapping.

[blackbeam]

I hope this was the last one :sweat_smile:

[accumulator]

Me too. Looking good now though! Thanks!