bugfix: Restrict content-length for compose box. #996
Conversation
There was a problem hiding this comment.
@zee-bit This looks to work quite well, though splitting out the model changes and adding a test over ZFL may be useful.
The other point which looks more important is that Zulip text (topics, content) is I believe typically stripped so that a becomes a. However, the readline limit is fixed - leading spaces in pasted text wouldn't disappear, but the end of it would be "blocked" by the new feature?
This issue aside, the content-length restriction seems like a good v1, but it doesn't give much feedback?
We can discuss, but having an explicit area colored as the edit area, or perhaps marking (not limiting?) the text beyond the limit may be some options to pursue?
|
It's worth noting that if we don't go for the full UI approach initially due to concerns there, or perhaps only for different input boxes, we could always have a popup or similar which asks the user what they wish to do with the extra text before sending, or similar. |
neiljp
added
the
PR awaiting update
zee-bit
force-pushed
the
restrict-stream-topic-length
branch
from
92743f6 to
e49dd2d
Compare
zulipbot
added
size: L
zee-bit
force-pushed
the
restrict-stream-topic-length
branch
2 times, most recently
from
698223f to
cb32209
Compare
There was a problem hiding this comment.
@zee-bit This is looking good as the first version - we did discuss other user feedback to explore in the future.
I do think we could later explore initial-whitespace trimming like in the search boxes if we're going to use this urwid feature, though we did have a debate then over the lack of feedback when typing whitespace and seemingly getting no response.
tests/ui_tools/test_boxes.py
Outdated
| @@ -26,6 +26,9 @@ def write_box(self, mocker, users_fixture, user_groups_fixture, | |||
| write_box = WriteBox(self.view) | |||
| write_box.view.users = users_fixture | |||
| write_box.model.user_dict = user_dict | |||
| write_box.model.max_stream_name_length = 60 | |||
| write_box.model.max_topic_length = 60 | |||
| write_box.max_message_length = 10000 | |||
There was a problem hiding this comment.
This typo (?) concerns me - do we even need these values?
There was a problem hiding this comment.
These are necessary for the tests to pass since the ReadlineEdit gets these values from model, and if we don't define this ourselves, Mock will be used in their place.
There was a problem hiding this comment.
I meant specifically the last line - which seems like a typo in that you have no model in there. Do mock values just work anyhow?
There was a problem hiding this comment.
Ahh...Right, my bad I missed that.
I just tested with mock values for all, and it fails as expected, for the above two but not for the max_message_length field. On looking at the test summary, it appears to fail because the above fields pass an edit_text parameter to the ReadlineEdit, but the msg_write_box does not. And, passing this parameter leads to the below condition in urwid_readline/readline_edit.py being positive, and max_char being of type Mock, can't be used as an index in slicing:
if max_char and "edit_text" in kwargs:
kwargs["edit_text"] = kwargs["edit_text"][:max_char]So, Mock values should work here if we don't specify edit_text parameter to ReadlineEdit initially. I have updated the PR fixing the above typo.
The server added a few content length restriction for compose boxes in version 4, feature_level 53. This commit allows for backporting these to older server versions, by using pre-defined values for these parameters, and storing them in Model for easier access by UI modules. Tests added.
zee-bit
force-pushed
the
restrict-stream-topic-length
branch
from
cb32209 to
22f83c8
Compare
This commit restricts input beyond certain max-limit for compose box, as specified by the corresponding parameters in initial_data's realm object. ZT didn't had this restriction earlier which resulted in certain long topic names/messages getting cropped from the end without the user noticing, until the message was sent. The content restriction of compose boxes was made possible only after urwid-readline merged a patch in v0.13, which added support for specifying a max_char argument to ReadlineEdit widget. Related patch: https://github.com/rr-/urwid_readline/pull/18. Tests ameded.
zee-bit
force-pushed
the
restrict-stream-topic-length
branch
from
22f83c8 to
01b47f7
Compare
|
@zee-bit Thanks for noticing and fixing this - just merging now 🎉 Could you file a follow-up issue for ideas we have so far around improving this? We may want to discuss further before proceeding. The numbers we have from this have relevance to aspects like #905 (compose header lengths), which could also relate to the new issue, as well as later perhaps things like maximum left panel widths, or the maximum search length? We don't have information for names (& emails for the current UI), and I'm not sure if the server intends to extend the coverage to include that level of parametrization, but that could open up scope for applying the same thoughts to the right panel width and compose area lengths. |
|
|
||
| model._store_content_length_restrictions() | ||
|
|
||
| if to_vary_in_initial_data: |
There was a problem hiding this comment.
This kind of branching suggests we could try hoisting values into the parametrizations, but this is fine.
neiljp
added
enhancement
This PR restricts input beyond certain max-limit for compose
box, as specified by the corresponding parameters in initial_data's
realm object. ZT didn't had this restriction earlier which resulted
in certain long topic names/messages getting cropped from the end
without the user noticing, until the message was sent.
These parameters were recently added in ZFL 53 in
4a3ad0d, so we allow forbackporting by defining hard-coded values for these parameters.
The content restriction of compose boxes was made possible only
after urwid-readline merged a patch in v0.13, which added support
for specifying a
max_charargument to theirReadlineEditwidget.Related patch: https://github.com/rr-/urwid_readline/pull/18.