-
Notifications
You must be signed in to change notification settings - Fork 31
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
Tasks archiving support in CHelper style #135
base: master
Are you sure you want to change the base?
Conversation
Hi, @AntKirill Thanks for your interest and for the PR. |
text="Restore Tasks" | ||
icon="/name/admitriev/jhelper/icons/unarchive.png" | ||
description="Restore directory with tasks or a single task back to the former place"> | ||
<add-to-group group-id="ProjectViewPopupMenu" anchor="first"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is is possible to add it to menu only for files for which it make sense (e.g cpp&xml & directories)? and ideally only inside project that work with JHelper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, but it puts a big overhead if we recursively check all the directories and inner files on every menu popup opening. To optimize, we can mark parent directories after the creation of a new JHelper config or a newly archived file, but that looks a bit complicated for now :) What do you think?
As a workaround, we can remove those buttons from the popup menu completely (a user can add them there manually anyway) and have them available via Ctrl+Shift+A
P.S. Thanks a lot for the very quick reply and for the awesome JHeleper plugin!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably would not check recursively, but I'd only add it for cpp&xml files and for directories (if that's possible). Others are guaranteed to not do anything.
Also, I think that adding it to the first position in the menu may be a little to aggressive and we should add it a bit lower.
Workaround you mentioned is always an option, it's what is done with other actions anyway
} catch (IOException ex) { | ||
throw new NotificationException("Unable to create archive directory", "Root of archive directory does not exist and failed to be created"); | ||
} | ||
VfsUtilCore.visitChildrenRecursively(selectedFile, new VirtualFileVisitor<>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it work ok if there are symlinks inside the directory? If there's a symlink to somewhere out of project, e.g /
, will it be able to wipe all my cpp's ? What about recursive symlinks?
} | ||
}); | ||
if (archivedCount > 0) { | ||
LocalFileSystem.getInstance().refresh(true); // to notify Vfs about new files created in archive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we notify fs if something was created even if there was exception in the middle?
"All tasks in " + selectedFile.getName() + " were successfully archived", | ||
NotificationType.INFORMATION | ||
); | ||
DeleteTaskAction.selectSomeTaskConfiguration(RunManagerEx.getInstanceEx(project)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange that we are going to different action here. Probably need to extract this soewhere. IDEUtils maybe
@@ -40,6 +41,11 @@ public ConfigurationDialog(@NotNull Project project, Configurator.State configur | |||
configuration.getRunFile(), | |||
RelativeFileChooserDescriptor.fileChooser(project.getBaseDir()) | |||
); | |||
archiveDirectory = new FileSelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes sense to store/find absolute path here?
Archived files do not really "participate" in the project anyway, and it'll allow less concatentation back and forth. (and it'll require less changes when they finally remove project.getBaseDir())
On the other hand it will make it less consistent and also make it harder to move th project across the FS.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, perhaps we can try to support both options by giving a special configuration item for the archive directory? It could look somehow like CardLayout component in swing. I can try to add this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's worth to support 2 options.
Do you know how CLion/IDEA handle paths themselves?
@@ -166,6 +167,7 @@ private static Test readTest(Element element) { | |||
|
|||
@Override | |||
public void writeExternal(Element element) { | |||
element.setAttribute("name", getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that name is already saved automatically. Can you please explain a bit more how it's used?
<component name="ProjectRunConfigurationManager">
<configuration default="false" name="A1: Consistency - Chapter 1" type="name.admitriev.jhelper.configuration.TaskConfigurationType" className="A1ConsistencyChapter1" cppPath="tasks/A1ConsistencyChapter1.cpp" inputType="LOCAL_REGEXP" inputFile="consistency_chapter_.*input[.]txt" outputType="CUSTOM" outputFile="consistency_chapter__output.txt" testType="MULTI_NUMBER">
<tests>
<test input="6 ABC F BANANA FBHC FOXEN CONSISTENCY " output="Case #1: 2 Case #2: 0 Case #3: 3 Case #4: 4 Case #5: 5 Case #6: 12 " active="true" />
</tests>
<method v="2" />
</configuration>
</component>```
return false; | ||
} | ||
}); | ||
UIUtils.openMethodInEditor(project, (OCFile) Objects.requireNonNull(PsiManager.getInstance(project).findFile(lastGeneratedFile)), "solve"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably not wise to throw here if there's no solve method
private void saveRCToArchive(TaskConfiguration taskConfiguration, VirtualFile directoryInArchiveForTheTask, String archiveRCFileName) { | ||
try { | ||
String rcArchiveFqn = directoryInArchiveForTheTask.getPath() + "/" + archiveRCFileName; | ||
Element rc = new Element("RC"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like here we do something very similar to what IDE does for us when saving configurations(in .idea/runConfigurations/
). Maybe it's possible and makes sense to reuse it somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd spell RC fully in the XML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tried to do this via their API, but it is not obvious how to make the platform "forget" about the RC stored in a separate file before deleting them. They have a handy method for that in platform com.intellij.execution.impl.RunManagerImpl#removeConfigurations$intellij_platform_execution_impl but it is private.
Alternatively, we can get a path to the file where our configuration is currently stored and just copy the file back and forth. But that seems to be worse since the platform may update the place where RCs are stored in the future and that will break restoring.
Do you see any other options to reuse the platform?
I agree about the spelling, I will fix it
Oh wow, This is actually better than my implementation. |
An attempt to support the feature Archive any task #120 in a different way than in PR.
The idea is to avoid a mess in RCs when working on a large number of tasks.
For example:
Features: