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

Fix "Move Section Contents to Seperate File" intention #2739

Merged
merged 19 commits into from
Nov 7, 2022

Conversation

jojo2357
Copy link
Contributor

@jojo2357 jojo2357 commented Nov 2, 2022

Fix #2724

Summary of additions and changes

  • Fixes bug outlined in Move section contents to separate file intention has unexpected bahvior with chapters with sections #2724
  • Changed the semantics behind nl.hannahsten.texifyidea.inspections.latex.codestyle.LatexTooLargeSectionInspection#findNextSection(LatexCommands) to require a section/chapter. I don't think it is the best, but I am unsure what else it is used for. If the function is only used to find the next section, from a section, then there will be no issue.
  • Note there is no guard to verify that the passed command is actually a valid section. I did this because I wanted to steer away from backsearching for one, since that requires more intimate knowledge of the usage than a basic "Find Usages" will provide.

How to test this pull request

\documentclass[11pt]{article}

\usepackage{amsmath}

\begin{document}

    \chapter{Foo}


    \section{SubFoo}
    Bar

    \subsubsection{SubbFoo}
    Barr

    \section{SubFooToo}


    \chapter{Too}


    \section{SubToo}
    \begin{equation}
        mathisfun
    \end{equation}

\end{document}

To test, take the above test file, and use the Move Section Contents to new File inspection. Perform this on every section and chapter and verify it works as expected. After running the inspection, undo the inspection, then try a second one. After each one works individually, try combinations thereof.

Now it will scan ahead to the next terminating command instead of only looking one ahead. This had the issue of *any* command being able to get in the way and force the rest of the file to be gobbled up, now it should work as intended on Hannah-Sten#2724
Now it will only consider commands that appear *after* the current command. It will now only consider commands ahead of the one passed, but right now requires that a chapter or section is passed.
Copy link
Collaborator

@PHPirates PHPirates left a comment

Choose a reason for hiding this comment

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

Awesome, great work!
I verified manually that it works, could you also add a test to avoid breaking it again in the future?

You can write a test for LatexMoveSectionToFileIntention, similar to the test LatexAddLabelIntentionTest


By the way, while reading the code I found another bug (not from this PR), consider this situation in an included file:

\section{Short}
Carefully dealing with the platforms as proceded by Maurice could tempt in governments.
\section{blub}
\begin{center}
    Units can be able to measure the protections, days mailed by performances, and resolve the cabinets since the attentions and the implication.
\end{center}
All correlations cleared by the reported penalties can be traded by the media.

Executing the intention on the second section will result in

\section{Short}
Carefully dealing with the platforms as proceded by Maurice could tempt in governments.
\section{blub}
\input{blub}

\end{center}
All correlations cleared by the reported penalties can be traded by the media.

If you could fix this as well, that would be really great.

@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 3, 2022

By the way, while reading the code I found another bug (not from this PR), consider this situation in an included file

Could not replicate the unexpected behavior with the test provided, or some variations thereof.

\documentclass[11pt]{article}
\begin{document}
    \section{Short}
    Carefully dealing with the platforms as proceded by Maurice could tempt in governments.
    \section{blub}
    \begin{center}
        Units can be able to measure the protections, days mailed by performances, and resolve the cabinets since the attentions and the implication.
    \end{center}
    All correlations cleared by the reported penalties can be traded by the media.
\end{document}

If I have misunderstood or otherwise am being dumb, please advise with a complete testcase

I will see about writing tests, and also about when undoing the intention also undoing the file creation

@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 3, 2022

Undoing is somehow not supported here. My hunch is that because we go behind IDEA's back with java.io.File#createNewFile and so are impervious to undo's

@PHPirates
Copy link
Collaborator

Try without all the extra lines you added, just put in the file what I wrote, and nothing more:

\section{Short}
Carefully dealing with the platforms as proceded by Maurice could tempt in governments.
\section{blub}
\begin{center}
    Units can be able to measure the protections, days mailed by performances, and resolve the cabinets since the attentions and the implication.
\end{center}
All correlations cleared by the reported penalties can be traded by the media.

@PHPirates
Copy link
Collaborator

Undoing is somehow not supported here. My hunch is that because we go behind IDEA's back with java.io.File#createNewFile and so are impervious to undo's

I can imagine that we would have to implement the undo ourselves. It does seem recommended to use File: https://plugins.jetbrains.com/docs/intellij/virtual-file.html#how-do-i-create-a-virtual-file

@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 3, 2022

Try without all the extra lines you added, just put in the file what I wrote, and nothing more

Verified, thank you

@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 3, 2022

I can imagine that we would have to implement the undo ourselves. It does seem recommended to use File

Then what is this doing with PsiFileFactory#createFileFromText(String, FileType, CharSequence)?

@PHPirates
Copy link
Collaborator

That's a PsiFile, not a VirtualFile. A PsiFile is like a parsed file: https://plugins.jetbrains.com/docs/intellij/psi-elements.html
You could try that of course if it helps

@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 3, 2022

I think I will because i found template files being created that way. If it doesn't work then you wont see it 🙃

Created Files#getUniqueFileName(String, String?). Should have no impact on existing code, but allows creating the smallest unique file.
It took a lot of hoop-jumping, but now MoveSectionToFile can be undone which will properly undo file creation. It will also use the first availible filename.
Fixes the issue of extraction when the last end command is not also EOF
Fix bugs for unit tests and improved stability
@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 4, 2022

Attempting to replicate the issue on my windows 10 install. so far unable to do so; seems like the ci needs more ram

@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 4, 2022

Fixed the issue you mentioned. Note that I am awaiting your blessing before applying the same undo-ability for the sister intention MoveSeLectionToFile. Turns out that it was possible, albeit with some inspiration from elsewhere

Copy link
Collaborator

@PHPirates PHPirates left a comment

Choose a reason for hiding this comment

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

The undo works great, amazing! Thanks for adding tests as well.

  • When I change the filename in the dialog, the created file will not have that name (it will be the default)
  • Now that the file creation code is larger (and better), can you check if you can reuse this for the quickfix from LatexTooLargeSectionInspection?

I can take a look at the memory issue in GH Actions

Create a function that can be reused to create a file without java.io
@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 5, 2022

Except for possibly fixing #2745 I believe this to be ready to go.

File not found now can handle absolute and relative paths better.
@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 5, 2022

Last commit helps address some of the issues brought up

@jojo2357 jojo2357 mentioned this pull request Nov 5, 2022
2 tasks
@jojo2357 jojo2357 requested review from PHPirates and removed request for stenwessel, slideclimb and HannahSchellekens November 5, 2022 18:38
@jojo2357
Copy link
Contributor Author

jojo2357 commented Nov 6, 2022

Huh this build failure seems warranted from the log unlike from earlier, gotta look into that then

Resolves the weird sectiontoolarge issue
Copy link
Collaborator

@PHPirates PHPirates left a comment

Choose a reason for hiding this comment

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

I think everything works now, awesome! Many thanks for fixing the other issues as well :)
(I didn't check but I hope it works on Windows as well)

@PHPirates PHPirates merged commit 4d625c9 into Hannah-Sten:master Nov 7, 2022
@jojo2357 jojo2357 deleted the 2724 branch November 7, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move section contents to separate file intention has unexpected bahvior with chapters with sections
2 participants