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

chore: clean up insertion marker code #6602

Merged

Conversation

rachel-fenichel
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Part of #6548

Proposed Changes

There are several rounds of changes in here. I did my best to have separate commits for basic naming changes, whitespace changes, and actual changes. I suggest reviewing one commit at a time.

The first bunch are fairly mechanical.

  • Move documentation comments out of the constructor and onto property declarations. This should also improve tsdoc.
  • Remove underscores from private property names
  • Remove underscores from private properties and methods
  • Format/lint

After that things get more complicated. The code previously had many checks for null that happened because a property could be null at some point, even though the relevant method was only called once and the relevant properties would never be null. Also, the information about the potential connection to make (the candidate connection) was stored in two separate properties, but hey always changed together. The code assumed that in multiple places before we had strict null checks. After turning on strict null checks, we needed lots of unnecessary checks.

How I resolved this:

  • When no candidate is found, return null from getCandidate instead of a CandidateConnection object with null properties. As a result, when anything non-null is returned, I know the properties it contains are also non-null (and so does the compiler).
  • Store the CandidateConnection as a single object, rather than storing the localConnection and closestConnection as separate properties. This means I don't have to coerce the compiler into believing that if one is non-null, the other is also non-null.
  • Pass values into methods when I know they are non-null, instead of accessing them as properties.

After that, I did assorted cleanup on the new code, including removing some errors that said things like "the code should never get here but the compiler insists that it could."

Behavior Before Change

No change.

Behavior After Change

No change.

Reason for Changes

Part of general tidying made possible by knowing which properties are internal and which are external.

Test Coverage

I wrote tests for this in #6596 and made sure they worked with the previous implementation and also worked after my changes.

I also opened up the advanced playground and manually looked at cases where the block would be inserted vs replaced, because the tests check whether a connection would be made, but not what happens to the previously connected block.

I don't have tests for inserting vs replacing because that's internal to the insertion marker. I could look at the state of the workspace after running applyConnections, though, if I trust my test setup enough. 🤔

Documentation

None

Additional Information

@rachel-fenichel rachel-fenichel requested a review from a team as a code owner November 3, 2022 21:44
@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Nov 3, 2022
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

The only mandatory change is are removing the extra whitespace in the JSDoc for hidePreview and fixing the JSDoc for maybeShowPreview.

core/insertion_marker_manager.ts Outdated Show resolved Hide resolved
core/insertion_marker_manager.ts Outdated Show resolved Hide resolved
core/insertion_marker_manager.ts Show resolved Hide resolved
@@ -443,14 +437,14 @@ export class InsertionMarkerManager {
* @returns Whether dropping the block immediately would delete the block.
*/
private shouldDelete(
candidate: CandidateConnection, dragTarget: IDragTarget|null): boolean {
candidate: CandidateConnection|null, dragTarget: IDragTarget|null): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like candidate is only being used for its truthiness. Why not make it a boolean and update the JSDoc to more clearly explain what it means as such?

If not then I think the JSDoc could still be improved, because repeating the definition of the CandidateConnection type is not very informative.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

core/insertion_marker_manager.ts Outdated Show resolved Hide resolved
core/insertion_marker_manager.ts Outdated Show resolved Hide resolved
core/insertion_marker_manager.ts Outdated Show resolved Hide resolved
core/insertion_marker_manager.ts Outdated Show resolved Hide resolved

/**
* Whether the block would be deleted if it were dropped immediately.
* Updated on every mouse move.
*/
private wouldDeleteBlock_ = false;
private wouldDeleteBlockInternal = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this property name, especially the Internal bit which suggests it is an internal implementation/alternative to the wouldDeleteBlock method, but it turns out not to be a method but a boolean.

Does the wouldDeleteBlock method, being marked @internal serve any useful purpose? Could it be removed, this property renamed wouldDeleteBlock, and calls to the former method replaced with property accesses? (There might even be a marginal performance benefit in avoiding method calls in what I am guessing is performance-sensitive code.)

If changes to public @internal properties are considered to be breaking (because someone might have ignored the @internal tag), then consider rename this to deleteBlockOnDrop or the like.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went with wouldDeleteOnDrop.

re: property access vs performance benefit: it's essentially acting as a getter right now, which is nice because alternate implementations could change the implementation of wouldDeleteBlock. But when I say it out loud, that sounds a lot like designing for a problem we haven't hit.

@maribethb my position on public @internal is that it's not breaking to change them, because during migration we removed @internal from things that were actually public, and anything left is actually @internal. Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, things marked @internal should not be considered public and therefore you can change them without it being breaking.

I agree with Christopher's suggestion to remove the get method and just use a property. Re: alternate implementation, is there actually a way to have an alternate implementation (e.g. can you register a custom insertion marker manager?)? if not, then that is more reason why it's not a problem we need to solve, and if so then we actually need to be careful because this might be protected rather than internal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

core/insertion_marker_manager.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Since you have fixed the (trivial) mandatory issues, I'm marking this approved, but I understand you're still considering the suggested changes to the un-highlighting code, and I'd be happy to re-re-review if you do so.

@rachel-fenichel
Copy link
Collaborator Author

Okay, I've made the changes we discussed for removing previews. PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants