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

unify initing processes of modes #3867

Closed
wants to merge 1 commit into from

Conversation

gdh1995
Copy link
Contributor

@gdh1995 gdh1995 commented Jul 18, 2021

There's a bug that some new-ed modes are never inited, imported when migrating from CoffeeScript to ES6. This PR fixes this by forcing subclasses of Mode to call super.init() in their constructors.

This fixes #3865, fixes #3866, fixes #3926, fixes #4062 and replaces #3877.

@primeapple
Copy link

Hey, what is needed to get this merged?

@philc
Copy link
Owner

philc commented Jan 20, 2022

@gdh1995 nice job finding the regression. I want to look at this more carefully, so I didn't bundle it with the release for #3985.

@gdh1995
Copy link
Contributor Author

gdh1995 commented Jan 20, 2022

It's OKay. Then I wish Vimium can merge #4064 to fix V and c in VisualMode (#3877 has some unrelated commits, so #4064 seems better).

@mrGrochowski
Copy link

@philc bump. Carret mode needed

@Pytness
Copy link

Pytness commented Aug 29, 2022

Bump.

Is there any reason a 1 year old pr that fixes a bug is not merged yet?

@AndyJado
Copy link

bump.

plz?

@philc
Copy link
Owner

philc commented Oct 17, 2022

Alright folks, sorry for the delay. @gdh1995 checking in again to see what you think of this vs. #4064 (which is also now equivalent to #3877)?

The use of inheritance in the implementation of modes is unnecessarily complex and I'd like to greatly simplify it at some point. #4064 is a simpler patch (3 LOC change) because it doesn't introduce new constraints around using super. I think they both fix precisely the same set of bugs, correct?

@gdh1995
Copy link
Contributor Author

gdh1995 commented Oct 17, 2022

Yes it worths to merge #4064.

@philc
Copy link
Owner

philc commented Oct 17, 2022

Ok great -- closing this in favor of #3877.

@philc philc closed this Oct 17, 2022
@gdh1995
Copy link
Contributor Author

gdh1995 commented Oct 17, 2022

BTW, I've reviewed Vimium's current code and noticed that:

  • Vimium's sub-classes of Mode overrides either constructor or init, so it is indeed acceptable to keep this usage.

@gdh1995 gdh1995 deleted the init-visualmode-correctly branch October 17, 2022 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants