Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show a summarised snippet of the latest reply from each thread #161

Closed
wants to merge 1 commit into from

Conversation

timruffles
Copy link
Contributor

screen shot 2018-11-11 at 15 58 25

Resolves #135

@@ -26,9 +26,11 @@ def make_engine(home: str):

class WithContent():

data = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a class variable? Shouldn't this be an instance variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to ensure all instances have a .data field throughout their life-time - I was getting instance has no attribute 'data' at times otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so you should do something like

class WithContent:

    def __init__(self, *nargs, **kwargs) -> None:
        self.data = None
        super().__init__(*nargs, **kwargs)

This makes sure every instance has a data field. Though if a class with any fancy metaclasses subclasses this, it may break. But the solution you have will cause problems if more than one instance of this class is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it was a mutable value I could see how this pattern could cause issues. As it’s None, from where would the problems you see stem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Python’s secretly prototypal object system - https://eev.ee/blog/2017/11/28/object-models/

@heartsucker
Copy link
Contributor

Also I have been reminded that we need to html.escape the content here.

@heartsucker
Copy link
Contributor

Hey @timruffles are you still working on this? If not, we can take over and make a couple of quick changes to get this finished up.

@timruffles
Copy link
Contributor Author

Please feel free to take it over! Probably won’t have time for a while

@heartsucker
Copy link
Contributor

On it :)

1 similar comment
@heartsucker
Copy link
Contributor

On it :)

@eloquence
Copy link
Member

eloquence commented Nov 30, 2018

There are a few issues with this from a UX perspective at present:

  1. When the text exceeds the width of the source list, it causes the source list to require horizontal scrolling. To reproduce, create a source with a message like "Everything in one line. Everything in one line. Everything in one line. Everything in one line."

  2. This PR implements a feature that's not in the spec -- a "↳" is inserted before messages that originate from the source (as opposed to replies). That's an interesting idea, but if we add a feature like that, we'll want to do some more thinking about doing it in a more intuitive way. Note that Signal, for example, does not indicate the provenance of snippets and generally relies on the user's memory for that.

  3. When a file is submitted after a message, only the placeholder "↳" is shown. Instead, per the spec, the most recently received message should be shown.

  4. When all we have is a single file (no later messages or replies), contrary to the specification, no text "—submitted a file, no message—" is shown as a snippet.

  5. When the text contains many newlines, it can take up to the entire height of the source list, or more.

Suggested minimal amendments from a UX perspective before merging:

  1. Wrap text to two lines at a width that corresponds to the width of the source widget.

  2. Remove ↳ feature for now.

  3. Swallow line breaks and replace them with spaces:

a.

b

c

becomes:

a. b c
  1. Show most recent message or reply if most recent submission is a file.

  2. Implement "single file, no messages or reply" behavior

Finally, note that after downloading a file, its preview snippet is replaced with <Content deleted>. I am not listing this above because files should be special-cased, anyway, per the above. NB: I stepped through this in detail with @ninavizz and she agrees with these recommendations.

@joshuathayer
Copy link
Contributor

Closing in favor of #214

legoktm pushed a commit that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants