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

Timeline: restore Item#message height shrink ability #612

Merged
merged 2 commits into from Jul 4, 2019

Conversation

rpallai
Copy link
Contributor

@rpallai rpallai commented Jul 3, 2019

This got broken by dbda393 which introduced circular dependency on height: Item#message height was dependent on childrenRect.height but it included a MouseArea which was dependent on parent.height.

It turned out by 87b18d9 via changing discardButton type from ActiveLabel to ToolButton which is taller and forces sent messages to shrink when it gets invisible (in normal circumstances, shortly after it appears).

@KitsuneRal
Copy link
Member

Now that I look once again at the code, I start wondering if we really need that many TimelineMouseAreas.

  1. Does it make sense to consolidate similar mouse areas into one under the top-level Item with anchors.fill: fullMessage instead of having one mouse area per each piece of the message? (and then this PR would be unnecessary in its current form)
  2. We now have 3 TimelineMouseAreas under textField - I strongly suspect we can consolidate them into one.

This got broken by dbda393 which introduced circular dependency on
height: `Item#message` height was dependent on
`childrenRect.height` but it included a `MouseArea` which was
dependent on `parent.height`.

It is fixed by consolidation of three MouseAreas into one under the
top-level `Item`. This was suggested by Kitsune Ral.
It's a practical simplification which was pointed out by Kitsune
Ral in PR#612.
@rpallai
Copy link
Contributor Author

rpallai commented Jul 4, 2019

  1. Does it make sense to consolidate similar mouse areas into one under the top-level Item with anchors.fill: fullMessage instead of having one mouse area per each piece of the message? (and then this PR would be unnecessary in its current form)

You are absolutely right. The reason of having different MouseAreas was historical but the code was reworked several times since and it isn't necessary anymore. I forgot to consolidate them. Thanks!

  1. We now have 3 TimelineMouseAreas under textField - I strongly suspect we can consolidate them into one.

I agree, it's a practical simplification. Pushed.

@KitsuneRal KitsuneRal merged commit d348115 into quotient-im:master Jul 4, 2019
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.

None yet

2 participants