Summary

Make the scale and moveState attributes of RegionCommentBlockView public.

Review Request #13986 — Created June 18, 2024 and submitted June 24, 2024, 7:18 a.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When converting the RegionCommentBlockView class to TypeScript, we made all

of its attributes private. With the doc review work in Power Pack, we'll have

a child class of RegionCommentBlockView for document region comments. We

want to be able to access the scale and move state attribute from this

child class, but we can't do this since they've become private on RB7.
This reverts back to how we had them before, renaming them from private

variables to their old _scale and _moveState names thus making them

public. This also means that for doc review region comments in Power Pack,

we don't have to deal with variable name compatibility, since the _scale

and _moveState name is used across RB5, 6, and 7.0.1.
One thing to note is that this means Power Pack 6 doc review region comments

will be broken on Review Board 7.0, they'll only be supported on 7.0.1+.

And, Power Pack 5.2.4's PDF review comments are only supported on 7.0.1+.

These comments are currently broken for 7.0. anyways.

Testing Done

Ran JS unit tests.

Tested created/moving/resizing/deleting PDF region comments.

Tested created/moving/resizing/deleting image region comments.

Commits

Summary	ID
Make the scale and moveState attributes of RegionCommentBlockView public.	459412d09d5636d11790e47e57493c06c763c76b

Issues

Description	From	Last Updated
I think if Power Pack needs this information (and _scale), we should just go ahead and make them officially public. …	chipx86	June 19, 2024, 1:09 p.m.
Let's use :js:attr: for these.	chipx86	June 24, 2024, 7:16 a.m.

flake8 passed.

JSHint passed.

Change Summary:: Added more description about supported versions.
Description:: When converting the RegionCommentBlockView class to TypeScript, we made all
of its attributes private. With the doc review work in Power Pack, we'll have
a child class of RegionCommentBlockView for document region comments. We
want to be able to access the scale and move state attribute from this
child class, but we can't do this since they've become private on RB7.

This reverts back to how we had them before, renaming them from private
variables to their old _scale and _moveState names thus making them
public. This also means that for doc review region comments in Power Pack,
we don't have to deal with variable name compatibility, since the _scale
and _moveState name is used across RB5, 6, and 7.0.1.

One thing to note is that this means Power Pack 6 doc review region comments
~ will be broken on Review Board 7.0, they'll only be supported on 7.0.1+.
~ will be broken on Review Board 7.0, they'll only be supported on 7.0.1+.
+ And, Power Pack 5.2.4's PDF review comments are only supported on 7.0.1+.
+ These comments are currently broken for 7.0. anyways.

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 1)

The issue has been resolved. Show all issues

I think if Power Pack needs this information (and _scale), we should just go ahead and make them officially public.

We could also assign to the _-based names for backwards-compatibility, and add a comment stating that they're deprecated, just so older Power Pack releases will continue to work.

maubin June 19, 2024, 1:09 p.m.

Since older versions of Power Pack don't even access these _-based variables (only my new changes in 5.0.x and 6 do), I'll just update my Power Pack changes to try accessing the public variables, and if they aren't defined then fall back on accessing the _-based ones. Falling back on the _ will maintain compatibility with RB5 and 6, and accessing the public variables will maintain compatibility with 7.0.1+.
Keeping the _ variables around along with the new public ones doesn't really help in any way. I would still need to do the workaround mentioned above where I try the public variables first then fall back on the _ ones, so I think it's safe to just get rid of them.

chipx86 June 21, 2024, 4:18 p.m.

The one advantage to keeping the _ ones around is that older Power Pack releases wouldn't break with these changes, but it's also fair to say people need to upgrade Power Pack for 7.0.x.

Commits:

	Summary	ID
	Make the scale and moveState attributes of RegionCommentBlockView public.	5b383ce16dae4150158b813e3be4b6c5c46f8d2a
	Make the scale and moveState attributes of RegionCommentBlockView public.	459412d09d5636d11790e47e57493c06c763c76b

Diff:

Revision 2 (+108 -84)

Show changes

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

reviewboard/static/rb/js/reviews/views/regionCommentBlockView.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's use :js:attr: for these.
```

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

		When converting the `RegionCommentBlockView` class to TypeScript, we made all
		of its attributes private. With the doc review work in Power Pack, we'll have
		a child class of `RegionCommentBlockView` for document region comments. We
		want to be able to access the scale and move state attribute from this
		child class, but we can't do this since they've become private on RB7.

		This reverts back to how we had them before, renaming them from private
		variables to their old `_scale` and `_moveState` names thus making them
		public. This also means that for doc review region comments in Power Pack,
		we don't have to deal with variable name compatibility, since the `_scale`
		and `_moveState` name is used across RB5, 6, and 7.0.1.

		One thing to note is that this means Power Pack 6 doc review region comments
~		will be broken on Review Board 7.0, they'll only be supported on 7.0.1+.
	~	will be broken on Review Board 7.0, they'll only be supported on 7.0.1+.
	+	And, Power Pack 5.2.4's PDF review comments are only supported on 7.0.1+.
	+	These comments are currently broken for 7.0. anyways.