Summary

Avoid duplicate instantiation and rendering of review UIs in diffs.

Review Request #13534 — Created Feb. 12, 2024 and submitted Feb. 27, 2024, 8:24 a.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When rendering review UIs into diffs, we'd end up creating instances for
both the original and modified files, even if we had a UI capable of
rendering a diff. This change makes it so we look up the matching Review
UI classes first and check if it's diff capable. If so, we only create
it once and use it to render the diff. If we have some other case (such
as only having one side or the other, or having a file type mismatch),
we'll instantiate each review UI and render the column.

Testing Done

Looked at a diff that had attachments for binary filediffs. Saw that
review UIs were instantiated and rendered correctly.

Commits

Summary	ID
Avoid duplicate instantiation and rendering of review UIs in diffs. When rendering review UIs into diffs, we'd end up creating instances for both the original and modified files, even if we had a UI capable of rendering a diff. This change makes it so we look up the matching Review UI classes first and check if it's diff capable. If so, we only create it once and use it to render the diff. If we have some other case (such as only having one side or the other, or having a file type mismatch), we'll instantiate each review UI and render the column. Testing Done: Looked at a diff that had attachments for binary filediffs. Saw that review UIs were instantiated and rendered correctly.	7681857eefa65fa5f12849951fca7fbc1ba9eab2

Summary

Avoid duplicate instantiation and rendering of review UIs in diffs.

When rendering review UIs into diffs, we'd end up creating instances for both the original and modified files, even if we had a UI capable of rendering a diff. This change makes it so we look up the matching Review UI classes first and check if it's diff capable. If so, we only create it once and use it to render the diff. If we have some other case (such as only having one side or the other, or having a file type mismatch), we'll instantiate each review UI and render the column. Testing Done: Looked at a diff that had attachments for binary filediffs. Saw that review UIs were instantiated and rendered correctly.

7681857eefa65fa5f12849951fca7fbc1ba9eab2

Issues

Description	From	Last Updated
Would it be a lot of effort to add testing for this, to ensure the intended logic doesn't regress?	chipx86	Feb. 15, 2024, 10:59 p.m.
Can you add typing for these while here?	chipx86	Feb. 15, 2024, 10:58 p.m.
Needs to be Type[...] for now. Same below.	chipx86	Feb. 19, 2024, 6:44 p.m.

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

The issue has been dropped. Show all issues

Would it be a lot of effort to add testing for this, to ensure the intended logic doesn't regress?

david Feb. 15, 2024, 11 p.m.

This is still in flux with my plans to add support for letting ReviewUIs operate on FileDiffs. I'd like to wait to add any testing until after I'm confident it's not going to change a bunch.

reviewboard/reviews/views/diff_fragments.py (Diff revision 1)

The issue has been resolved. Show all issues

Can you add typing for these while here?

Commits:

	Summary	ID
	Avoid duplicate instantiation and rendering of review UIs in diffs. When rendering review UIs into diffs, we'd end up creating instances for both the original and modified files, even if we had a UI capable of rendering a diff. This change makes it so we look up the matching Review UI classes first and check if it's diff capable. If so, we only create it once and use it to render the diff. If we have some other case (such as only having one side or the other, or having a file type mismatch), we'll instantiate each review UI and render the column. Testing Done: Looked at a diff that had attachments for binary filediffs. Saw that review UIs were instantiated and rendered correctly.	869558bfd9dd3b22908f7e6c885eebe31a6df198
	Avoid duplicate instantiation and rendering of review UIs in diffs. When rendering review UIs into diffs, we'd end up creating instances for both the original and modified files, even if we had a UI capable of rendering a diff. This change makes it so we look up the matching Review UI classes first and check if it's diff capable. If so, we only create it once and use it to render the diff. If we have some other case (such as only having one side or the other, or having a file type mismatch), we'll instantiate each review UI and render the column. Testing Done: Looked at a diff that had attachments for binary filediffs. Saw that review UIs were instantiated and rendered correctly.	7681857eefa65fa5f12849951fca7fbc1ba9eab2

Diff:

Revision 2 (+62 -54)

Show changes

reviewboard/reviews/views/diff_fragments.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/views/diff_fragments.py (Diff revision 2)

The issue has been dropped. Show all issues

Needs to be Type[...] for now.
Same below.

david Feb. 19, 2024, 6:14 p.m.

Because this is in an annotation, it works as long as we make sure to have from __future__ import annotations. It's only things that aren't in annotations (such as inheriting from Registry[Type[...]]) that still need the typing version.

chipx86 Feb. 26, 2024, 10:40 p.m.

That's true, though I feel consistency is ideal, because otherwise it's easy to use one in place of the other in the wrong place.

I'm looking forward to eventually moving entirely to built-in types. Until then, it'd be nice to be consistent, but I don't want this to be a blocker for this change.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (9bd048f)