#6370 Add line numbers to sections and text-blocks

[ahmedyarub]

Checklist

• I have filed an issue about this change and discussed potential changes with the maintainers.
• I have received the approval from the maintainers to make this change.
• This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: 6370

Description of this change

Add line numbers to the text blocks and sections so that we can identify them from a PsiElement. It is obvious that not in all cases we know the line number: when we are generating a new file, the line numbers are only known when the file is actually generated. In these cases I set the line number to -1 knowing that before the object is used it will go through the print() command. There, I would update the line number of the element.
If at the moment of the change (ex in my target management feature I already know the line number of the directory entry to change), then there is no need to assign a dummy value first.

[blorente]

If we're using -1 to mark "uninitialized", could we make it a constant somewhere? I have a short memory, I'm sure I'll forget what -1 means 6months from now when I come back to this code.

[blorente]

Additionally, I'm a bit scared of printing having side effects on the data structure.
Could we instead traverse the data structure in ProjectView.Builder.build() and assign line numbers then? That way we make sure that:

• Printing doesn't modify the structure.
• We always have line numbers when we build a project view, whether we print it or not.

[blorente]

And, because I forgot to start with this: Thank you for your contributions! Most maintainers here have limited time to implement cool features in the plugin, and it's delightful to see the plugin improving.

[ahmedyarub]

If we're using -1 to mark "uninitialized", could we make it a constant somewhere? I have a short memory, I'm sure I'll forget what -1 means 6months from now when I come back to this code.

I was about to do that and somehow forgot about it! Done.

[ahmedyarub]

Additionally, I'm a bit scared of printing having side effects on the data structure. Could we instead traverse the data structure in ProjectView.Builder.build() and assign line numbers then? That way we make sure that:

• Printing doesn't modify the structure.
• We always have line numbers when we build a project view, whether we print it or not.

What you are mentioning is the ideal solution beyond any doubt. The problem is that each print statement decides on its own how many new lines to add. Moving the logic to the builder means consolidating all that "new-line logic" into the ProjectView class. Can we guarantee that future printers would be compatible with ProjectView? I mean we can at least add a comment to the print() method in the interface saying that line numbers should be changed when printing which would be easily visible for devs.

[ahmedyarub]

And, because I forgot to start with this: Thank you for your contributions! Most maintainers here have limited time to implement cool features in the plugin, and it's delightful to see the plugin improving.

I really appreciate your support and patience you and especially @tpasternak . I assure you that there will be much bigger changes to come.

[ahmedyarub]

OK so I removed line number calculation from all the print() methods and then tested the plugin behavior thoroughly. I found that all the code-paths call the parseProjectView() after writing to the file multiple times so there is no need to calculate the line numbers when the file is being created or edited.

[blorente]

Thanks for making the changes!

Given the flow you've proposed, I'm not sure we need TEMPORARY_LINE_NUMBER anymore. In fact, I'm not sure we need to pass line numbers to the individual methods of the Builders of different sections.

If I understand correctly, every creation and update to a ProjectView goes through a Builder. If we pass the initial line index to a Builder, it should have enough information to assign line numbers based on the operation and the sections it contains.

So, instead of having Builder.add(T element, int lineIdx), we'd have Builder.add(T element) and the Builder would take care of calculating which line numbers T should occupy.

I think this encapsulates the behaviour of adding line numbers neatly into the Builders, so that the calling code doesn't have to keep that state.

What do you think?

[ahmedyarub]

Thanks for making the changes!

Given the flow you've proposed, I'm not sure we need TEMPORARY_LINE_NUMBER anymore. In fact, I'm not sure we need to pass line numbers to the individual methods of the Builders of different sections.

If I understand correctly, every creation and update to a ProjectView goes through a Builder. If we pass the initial line index to a Builder, it should have enough information to assign line numbers based on the operation and the sections it contains.

So, instead of having Builder.add(T element, int lineIdx), we'd have Builder.add(T element) and the Builder would take care of calculating which line numbers T should occupy.

I think this encapsulates the behaviour of adding line numbers neatly into the Builders, so that the calling code doesn't have to keep that state.

What do you think?

I tried doing that but found multiple instances where the builder is instantiated and used without having known line numbers. The line numbers are mostly defined when the print() method is called, and that's why we are depending on reparsing to replace the temporary line numbers with real one. I can't think of a way for example to make that change to AddDirectoryToProjectAction.doOKAction() and GenerateFromBuildFileSelectProjectViewOption.guessProjectViewFromLocation(). How do you suggest doing these changes in these methods?

[ahmedyarub]

@tpasternak @blorente is there anything else that I need to do here?

[blorente]

I finally got a little bit of time to re-review this.
It seems we can't get more invariants from the codebase as it is, so I think it's fine to merge this and iterate later on how we can remove TEMPORARY_LINE_NUMBER.

Approving with the caveat that I'd love to see more tests, I'll be happy to merge once the tests are there.

[ahmedyarub]

@blorente is there any other requirement for merging?

[blorente]

Not as far as I can tell, thanks for the contribution, and apologies for the slow review turnarounds!