-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Let FileSystem extensions set filetype icons in filesystem browser #7062
Conversation
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.
See my inline code comments.
setIcon(icon); | ||
GFile gf = node.getGFile(); | ||
GFileSystem fs = gf.getFilesystem(); | ||
// TODO is it OK to pass a null monitor? |
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.
re: is it okay to use a null monitor?
answer: no. A good rule is that if a method needs a TaskMonitor, it shouldn't be used from the Swing thread.
A common pattern is for a method that runs on the swing gui thread to call TaskLauncher.launch[Non]Modal(name, monitor -> doTheThing(monitor) );
, and then sometimes at the end of doTheThing(TaskMonitor)
, it will call Swing.runLater( () -> updateSomeGuiThing(data_created_slowly_in_the_task_thread) );
For renderFile()
, FSBFileNode.updateFileProps(TaskMonitor)
is there for just this use-case, so query and save the file attribute value there, and then use that value here in renderFile()
.
// TODO is it OK to pass a null monitor? | ||
FileAttributes fa = fs.getFileAttributes(gf, null/*monitor*/); | ||
|
||
String ext = fa.get("typeExt", String.class, ""); |
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 would add a new FileAttributeType, maybe something like:
FILENAME_EXT_OVERRIDE("Extension override", MISC_INFO, String.class)
right after COMMENT_ATTR
, and then in FSBFileNode.updateFileProps(), query for it in a similar manner as the other values, and save it for later use.
Then, instead of adding a new fsbIcons.getIconByExt() method, seems like we could just smush the existing filename together with the extension override and then query the icon in the normal way.
A bit of a hack, but pretty clear and easy to understand and easy to 'fix' later on if we run into a requirement where we need more feature-rich file typing.
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.
additionally, there should be some logic that ensures the extension override value is formatted (or not formatted) with a leading dot '.', so that when its used to concat with the filename we get a good value, regardless of what the filesystem impl throws at us.
I believe I've addressed all of your concerns. I'm not a Java guy so I wasn't sure if I should also add a getter for I'm logging an error with (I'm still learning Git & Github so let me know if I'm doing anything wrong please.) |
re: getter for the filenameExtOverride value. Yeah, there should be one. The final step for this PR would be to squash your commits into 1 and then force-push overwrite your PR branch with that new commit, and then we'll merge that commit. |
afd213e
to
3943dd3
Compare
I struggle a lot with Git. I followed some guides to learn how to squash my commits but from looking at the above it looks wrong to me. Did I accidentally squash in the commit prior to my PR? Or is this just how GitHub makes it look? |
I think that is many of us. The concepts and interfaces are not as intuitive as I would like. Not sure I could design it any better. |
It does look suspicious because that previous commit from ghizard has your name as the committer, but I don't think any source code was comingled. However, it will prevent a normal PR merge from happening from our side. Its possible to fix your branch, but for something so small it may be easier for you to just create a new PR branch, manually migrate your code changes from the broken branch into the new one and then submit it again. |
2nd attempt: #7070 |
Fixed by 860915d |
Provides the feature I asked about in "Can a FileSystem extension set icons for files in the FileSystem viewer/browser?" #6953
This solution involves the fewest modifications to the existing code.
In a
FileSystem
'sgetFileAttributes()
method you just add atypeExt
attribute which is just a file extension including the dot, as in.txt
and in this way leverages the existing system which uses the file's extension to decide which icon to use.FileSystem
s which know the types of their files and which do not use the file extension convention can thus set the types of files:The mapping of file extensions to icons is at
Ghidra/Features/Base/data/base.file.extensions.icons.theme.properties
and the icons themselves are inGhidra/Features/Base/bin/main/images
(I would actually say a bunch of useful ones are still missing, such as
.exe
,.dll
,.o
, etc.)