[ui polish] Reply pane styling

[deeplow]

Do not merge. Waiting on discussion in #580 (comment)

Description

• Converts rich text into plaintext (reply box was accepting rich text input)
• add styling according to mockup

Fixes #539 and partially #572

Before:

After:

Objective: (mockup)

Test Plan

1. copy rich text
2. paste it into reply box

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

Comments

Considerations for future PRs:

• keyboard shortcuts (ctrl+return) in discussion if should be implemented: [ui polish] Reply pane styling/behavior #572
• add rich text input (or alternative) only for placeholder "Compose a reply to Source Name"
• think about the scrollbar (should it go away?)
• Shouldn't the text scale with the content? (it might be hard to have 20% of the screen with an empty reply box when the journalist is reading messages) Discussion: [ui polish] Reply pane styling/behavior #572

[deeplow]

Actually it might not be as simple as this because if we want to to the placehoder like compose a reply to Surreal Yo-Yo, it might require rich text but somehow get disabled.

I will investigate this.

[deeplow]

I will investigate this.

It was decided that this may go on another PR

[deeplow]

The client is crashing in offline mode. I'll check that later

[deeplow]

The client is crashing in offline mode.

this has now been fixed and some styling to the offline mode was added as well as a placeholder for online mode with the temporary text "Compose a reply to Bold Sourcename"

Some discussion on the offline mode styling is still happening in #572 (comment)

[deeplow]

I've hidden the vertical scrollbar in the reply box even when the box is bigger than its size.
@ninavizz let me know if this is something we want:

Before:

After:

[deeplow]

I'll fix the linting problem that is making the test fail on CI

[ninavizz]

Yes, thank you!! The little purple airplane should be a few pixels lower, though; it's lowest points should be flush with the baseline of the last line of text. Thank you, Deeplow!

[deeplow]

The little purple airplane should be a few pixels lower, though; it's lowest points should be flush with the baseline of the last line of text

Like this? (the image below is the mockup)

The only problem now is that it crops the last line in half.

Also, @ninavizz I would also like some feedback on the airplane in offline mode. Quoting from the issue

I didn't see any mockups for the offline mode, so I just did what I thought made sense.

The current version looks like this: (over in #580)

@ninavizz I think we should also add a disabled-looking airplane. What do you think? (it the last iterations of the code there were none). With the plane disabled it would look like:

I already have the code for that which I can merge into #580, in case that's the way to go.

[deeplow]

I'll be trying to add the source name in the reply box

[sssoleileraaa]

@deeplow the disabled-looking airplane looks great to me, we can discuss after standup to see if others are in agreement, but I think you should add that in.

also, the button looks giant now, but it does match the mockups so i don't think my opinion should hold up this pr. i'll bring it up to see what others think now that they can see it in the client application.

One change I think you should make in this PR is using the actual journalist_designation name instead of %bold_source_name% as a placeholder. If you'd rather create a separate PR for retrieving the name, then I recommend changing Compose a reply to %bold_source_name% to just Compose a reply.

Also, currently when clicking inside the textbox the Compose a reply text does not go away as I expected. Instead it goes away when you start typing, but I think it's distracting having the text in front of the cursor as you're thinking about what to type. It would be great if you could update this behavior as part of this PR as well.

[deeplow]

@creviera could you check if the test I did looks sane?

I had to make select a source from the sources list. Otherwise, I think it would be always true since I'd be creating a source and using that for a ReplyBoxWidget and testing the placeholder.

[deeplow]

Also, currently when clicking inside the textbox the Compose a reply text does not go away as I expected. Instead it goes away when you start typing, but I think it's distracting having the text in front of the cursor as you're thinking about what to type. It would be great if you could update this behavior as part of this PR as well.

@creviera regarding this, It's intended Qt behavior and there doesn't seem to be a default way to disable that. We might have to find another way, but let's do that in another PR, I think.

From the docs (https://doc.qt.io/qt-5/qtextedit.html#placeholderText-prop)

placeholderText : QString

This property holds the editor placeholder text

Setting this property makes the editor display a grayed-out placeholder text as long as the document() is empty.

[sssoleileraaa]

@creviera could you check if the test I did looks sane?

I had to make select a source from the sources list. Otherwise, I think it would be always true since I'd be creating a source and using that for a ReplyBoxWidget and testing the placeholder.

I think your test looks good, although to improve code clarity, you could change A->B:

A:

    sl = SourceList()
    sl.setup(controller)
    ...
    source_1 = factory.Source()
    source_2 = factory.Source()
    sl.update([source_1, source_2])
    source_1_item = sl.item(0)
    source_2_item = sl.item(1)
    ...
    sl.setCurrentItem(source_2_item)
    selected_source = sl.itemWidget(sl.currentItem()).source
    rb = ReplyBoxWidget(selected_source, controller)
    assert rb.text_edit.placeholderText().find(source_2.journalist_designation) != -1

B:

    source = factory.Source()
    rb = ReplyBoxWidget(source, controller)
    assert rb.text_edit.placeholderText().find(source.journalist_designation) != -1

Because all you're really testing is that the journalist_designation used in the placeholder text matches the journalist_designation of the ReplyBoxWidget source.

The rest of your test is better done at the SourceConversationWrapper level. That's where we do things like making sure the ReplyBoxWidget is initialized with the currently selected source whenever selection changes in the SourceList etc.

[sssoleileraaa]

Ah, looks like qt's placeholderText is not what we want here then. I agree that it makes sense to come up with a solution in a follow-up pr, especially if that solution is to add a QLabel to the ReplyBoxWidget that hides on a focus event and allows rich-text formatting for the source name -- solves two issues in one!