Opening a context menu jumps to the selected line

[nrc]

STR:

• jump to def on a page (or some other action) so that a given line is selected
• scroll up or down so the selected line is out of view
• open a context menu
• page jumps back to the selected line

[nrc]

Has to be a ref menu, not a line number menu. Closing the menu also jumps

[nrc]

This is because we calling componentDidUpdate when opening or closing a ref menu which calls jumpToLine

[nkanderson]

@nrc is componentDidUpdate being called when opening or closing a ref menu due to refMenu being in the state? Or is there somewhere explicit that's being called?

I'm also wondering what the reason is for calling componentDidUpdate from within componentDidMount - is it to initialize the source links?

[nrc]

is componentDidUpdate being called when opening or closing a ref menu due to refMenu being in the state?

Yes

I'm also wondering what the reason is for calling componentDidUpdate from within componentDidMount - is it to initialize the source links?

That and to add any highlights and to scroll to the correct line.

If we were to do this 100% properly, then the highlights should be a component and could be handled implicitly via render, and the source links too could probably be done inline. However, that is (I think) a big refactoring. I'm not sure about the best way to handle the scrolling, I think maybe DidUpdate and DidMount is the right place for that.

[nkanderson]

Given the constraints, I'm leaning towards adding a flag for whether or not the refMenu has changed in state, within componentDidUpdate. It would be added to the this.props.highlight conditional to determine whether or not jumpToLine should be called. Does that sound too clunky?

I was trying to think through ways to better tap into the component lifecycle, but not having the highlights as a component I think means we have to keep those conditional checks and behavior within componentDidUpdate.

[nkanderson]

So f22b6f7 is what I had in mind for adding a menuChanged flag, which would be used to determine whether or not jumpToLine should be called. It seems to solve the immediate issue, but there continue to be conflicts down the line with this.props.highlight and various components updating.

For example, if you open and close a menu, it should be fine, but if you then click on one of the src_link items, you'll jump to the previously set line_start. I'm not sure of the best way to handle this, since the highlight prop is set on a parent component.

Also, I'm not sure why that commit ID above is linking to the main repo instead of my fork, but I've got a branch w/ that commit on it in my fork ¯_(ツ)_/¯

[nrc]

How much work do you think it would be to factor out a highlight component? That seems like the 'right' thing to do here. I have shied away from doing it in the past just because it seemed hard, but maybe it is not too bad?

It might be possible to keep the line to jump to in the state rather than just a prop, then when we use set state to open a menu we also remove the line to jump to. Not sure exactly when we'd initialise the state from the prop though.

[nkanderson]

wrt a highlight component - I think it would be worth putting some time against, at least. I remember thinking at some point that there would be some additional issues that would be easier to address if highlights were their own component. I should be able to look into it more later this week, or this weekend.

[nkanderson]

Hi @nrc - I've been spending some time working on a highlight component, and I have a proof-of-concept working for a very basic case of a highlight in a single line.

What do you think is a good way to test multi-line highlights? I'd like to make sure I'm testing against a few different highlight cases, then see if this helps with the jumpToLine issues.

[nrc]

Hmm, I'm not sure we even use the multi-line highlights any more. They were used for error highlights, but with search and navigation, I think we only highlight single lines.

[nkanderson]

Ah, cool. In that case, I'll leave in the logic just in case it's still in use somewhere we forgot, but focus on testing with single-line highlights.