1117080 - Limit conversations (no Rooms) subject to 124 characters.

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P3)

Description

[Mark Banner (:standard8)]

Bug 1107082 implemented a limit on the number of characters for a room name. I think we should probably somehow reflect that in the UI - possibly one of:

- find out server limit
- hard-code the limit
- Allow the user to do any length, detect the server failure and reflect it in an informed way to the user.

Comment 1

[Mark Banner (:standard8)]

Sevaan: We should probably be doing client-side limiting of this even though the server limit added in bug 1107082 doesn't apply now.

Is 124 characters a reasonable number? (this was originally as suggested by the FxOS client, but I have no details as to why it was that number).

Comment 2

[Maria Angeles Oteo (:oteo)]

(In reply to Mark Banner (:standard8) from comment #1)

> Is 124 characters a reasonable number? (this was originally as suggested by
> the FxOS client, but I have no details as to why it was that number).

No reasons from our FxOS client (at least from a UX point of view), Alexis suggested that number and Product team was fine with it (see https://bugzilla.mozilla.org/show_bug.cgi?id=1095358#c2)

Comment 3

[Sevaan Franks [:sevaan]]

124 seems aribitrary yet reasonable. In the panel or room titlebar, the conversation name should trail off to elipses if it is too long.

I don't think we need to limit the number of characters to force room titles to fit.

As such, we can't just leave it open-ended in-case someone puts in a gigantic string which causes problems.

We can leave it as it, or even make it Twitter limit for fun: 140 characters.

Comment 4

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: snapshot102_chars.png

Setting 102 characteres to the converstaion name, server rejects renaming, so the practical limit is 101

Comment 5

[Mark Banner (:standard8)]

That's strange. What we save on the server is encrypted context - there shouldn't be a length limit at all.

Comment 6

[Mark Banner (:standard8)]

Hang on a minute, I think that's a screenshot of release which isn't using the context yet. Can you try nightly instead?

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Bug1117080.patch

Comment 8

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Mark Banner (:standard8) from comment #6)
> Hang on a minute, I think that's a screenshot of release which isn't using
> the context yet. Can you try nightly instead?

This version had been built by me this morning

Comment 9

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Bug1117080.patch

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Mark Banner (:standard8) from comment #5)
> That's strange. What we save on the server is encrypted context - there
> shouldn't be a length limit at all.

My last test (with patch) stored a 124 name:

1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234

Comment 11

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Bug1117080.patch

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8632098 [details] [diff] [review]
Bug1117080.patch

Review of attachment 8632098 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 with the eslint error fixed. The other one is just a comment.

::: browser/components/loop/content/js/roomViews.jsx
@@ +285,5 @@
>    });
>  
>    var DesktopRoomEditContextView = React.createClass({
>      mixins: [React.addons.LinkedStateMixin],
> +    maxRoomNameLength: 124,

I found there was an instance where we could use "statics" as part of the createClass definition, but it seems mainly for functions and I doubt it'd by us much, so I think defining like this is fine.

@@ +512,5 @@
>            <form onSubmit={this.handleFormSubmit}>
>              <input className="room-context-name"
>                onKeyDown={this.handleTextareaKeyDown}
>                placeholder={mozL10n.get("context_edit_name_placeholder")}
> +              maxLength={this.maxRoomNameLength}

Eslint says:

516:14  error  Props should be sorted alphabetically  react/jsx-sort-props

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ce43b449ebe7

Comment 14

[Mark Banner (:standard8)]

Fernando: I've landed this for you with the change I asked for, as I know we're both away next week.

Comment 15

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Mark Banner (:standard8) from comment #14)
> Fernando: I've landed this for you with the change I asked for, as I know
> we're both away next week.

Thank you Mark !

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce43b449ebe7

Comment 17

[Bogdan Maris, Desktop QA]

Verified that conversation name can contain a maximum of 124 characters on latest Nightly across platforms (Windows 7 64-bit, Mac OS X 10.10 and Ubuntu 14.04 32-bit).