ida: add adjustable font menu in IDA plugin#2995
ida: add adjustable font menu in IDA plugin#2995vee1e wants to merge 3 commits intomandiant:masterfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed
|
@gemini-code-assist review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces user-configurable font settings for the capa IDA plugin, adding a font selection dialog and ensuring the chosen font is propagated across the main UI components, including the rule generator and data models. Review feedback highlights opportunities to refactor duplicated font-loading logic, improve the consistency of font application across all UI elements (such as search bars and status labels), and ensure that hardcoded labels also respect the user's font preferences.
| def load_font(self): | ||
| """load the user-configured font or fall back to the system fixed font""" | ||
| font_str = settings.user.get(CAPA_SETTINGS_FONT, "") | ||
| font = QtGui.QFontDatabase.systemFont(QtGui.QFontDatabase.FixedFont) | ||
| if font_str: | ||
| font.fromString(font_str) | ||
| self.update_fonts(font) | ||
|
|
||
| def slot_font(self): | ||
| """launch the font dialog and apply the chosen font""" | ||
| font_str = settings.user.get(CAPA_SETTINGS_FONT, "") | ||
| current_font = QtGui.QFontDatabase.systemFont(QtGui.QFontDatabase.FixedFont) | ||
| if font_str: | ||
| current_font.fromString(font_str) | ||
| font, ok = QtWidgets.QFontDialog.getFont(current_font, self.parent, "Select Plugin Font") | ||
| if ok: | ||
| settings.user[CAPA_SETTINGS_FONT] = font.toString() | ||
| self.update_fonts(font) |
There was a problem hiding this comment.
The logic for retrieving the font from settings and falling back to the system fixed font is duplicated in load_font and slot_font. Consider refactoring this into a private helper method to improve maintainability and ensure consistency in how fonts are initialized.
| def update_fonts(self, font: QtGui.QFont): | ||
| """propagate the selected font throughout the plugin UI""" | ||
| if hasattr(self, "model_data") and self.model_data: | ||
| self.model_data.update_font(font) | ||
| if hasattr(self, "view_tree") and self.view_tree: | ||
| self.view_tree.update_font(font) | ||
| if hasattr(self, "view_rulegen_preview") and self.view_rulegen_preview: | ||
| self.view_rulegen_preview.update_font(font) | ||
| if hasattr(self, "view_rulegen_editor") and self.view_rulegen_editor: | ||
| self.view_rulegen_editor.update_font(font) | ||
| if hasattr(self, "view_rulegen_features") and self.view_rulegen_features: | ||
| self.view_rulegen_features.update_font(font) |
There was a problem hiding this comment.
The update_fonts method propagates the font to the main data views but misses several other UI components, such as the search bars (view_search_bar, view_rulegen_search), status labels (view_status_label, view_rulegen_status_label), and the rule generator header labels. To ensure a consistent look and feel across the entire plugin, consider applying the font to the parent widget using self.parent.setFont(font) or explicitly updating these additional components.
| def update_fonts(self, font: QtGui.QFont): | |
| """propagate the selected font throughout the plugin UI""" | |
| if hasattr(self, "model_data") and self.model_data: | |
| self.model_data.update_font(font) | |
| if hasattr(self, "view_tree") and self.view_tree: | |
| self.view_tree.update_font(font) | |
| if hasattr(self, "view_rulegen_preview") and self.view_rulegen_preview: | |
| self.view_rulegen_preview.update_font(font) | |
| if hasattr(self, "view_rulegen_editor") and self.view_rulegen_editor: | |
| self.view_rulegen_editor.update_font(font) | |
| if hasattr(self, "view_rulegen_features") and self.view_rulegen_features: | |
| self.view_rulegen_features.update_font(font) | |
| def update_fonts(self, font: QtGui.QFont): | |
| """propagate the selected font throughout the plugin UI""" | |
| if hasattr(self, "parent") and self.parent: | |
| self.parent.setFont(font) | |
| if hasattr(self, "model_data") and self.model_data: | |
| self.model_data.update_font(font) | |
| if hasattr(self, "view_tree") and self.view_tree: | |
| self.view_tree.update_font(font) | |
| if hasattr(self, "view_rulegen_preview") and self.view_rulegen_preview: | |
| self.view_rulegen_preview.update_font(font) | |
| if hasattr(self, "view_rulegen_editor") and self.view_rulegen_editor: | |
| self.view_rulegen_editor.update_font(font) | |
| if hasattr(self, "view_rulegen_features") and self.view_rulegen_features: | |
| self.view_rulegen_features.update_font(font) |
| font = QtGui.QFontDatabase.systemFont(QtGui.QFontDatabase.FixedFont) | ||
| font.setBold(True) |
There was a problem hiding this comment.
This font is hardcoded to the system default FixedFont and will not reflect the user's font preference if they change it via the new "Font..." menu. For a consistent UI, these labels should also respect the configured plugin font.
There was a problem hiding this comment.
Code Review
This pull request introduces a feature allowing users to customize the font used throughout the plugin UI, including a new font selection dialog and propagation logic across various components. The review feedback suggests refactoring duplicated font-loading logic into a helper method, improving the maintainability of the font propagation loop in the main form, and adding missing docstrings to new methods for better consistency.
| def update_fonts(self, font: QtGui.QFont): | ||
| """propagate the selected font throughout the plugin UI""" | ||
| if hasattr(self, "model_data") and self.model_data: | ||
| self.model_data.update_font(font) | ||
| if hasattr(self, "view_tree") and self.view_tree: | ||
| self.view_tree.update_font(font) | ||
| if hasattr(self, "view_rulegen_preview") and self.view_rulegen_preview: | ||
| self.view_rulegen_preview.update_font(font) | ||
| if hasattr(self, "view_rulegen_editor") and self.view_rulegen_editor: | ||
| self.view_rulegen_editor.update_font(font) | ||
| if hasattr(self, "view_rulegen_features") and self.view_rulegen_features: | ||
| self.view_rulegen_features.update_font(font) |
There was a problem hiding this comment.
The update_fonts method manually checks for several attributes and calls their update_font method. This approach is repetitive and can be error-prone if new components are added in the future. Consider iterating over a list of component names to propagate the font update more cleanly.
| def update_fonts(self, font: QtGui.QFont): | |
| """propagate the selected font throughout the plugin UI""" | |
| if hasattr(self, "model_data") and self.model_data: | |
| self.model_data.update_font(font) | |
| if hasattr(self, "view_tree") and self.view_tree: | |
| self.view_tree.update_font(font) | |
| if hasattr(self, "view_rulegen_preview") and self.view_rulegen_preview: | |
| self.view_rulegen_preview.update_font(font) | |
| if hasattr(self, "view_rulegen_editor") and self.view_rulegen_editor: | |
| self.view_rulegen_editor.update_font(font) | |
| if hasattr(self, "view_rulegen_features") and self.view_rulegen_features: | |
| self.view_rulegen_features.update_font(font) | |
| def update_fonts(self, font: QtGui.QFont): | |
| """propagate the selected font throughout the plugin UI""" | |
| components = [ | |
| "model_data", | |
| "view_tree", | |
| "view_rulegen_preview", | |
| "view_rulegen_editor", | |
| "view_rulegen_features", | |
| ] | |
| for comp_name in components: | |
| comp = getattr(self, comp_name, None) | |
| if comp: | |
| comp.update_font(font) |
| def update_font(self, font: QtGui.QFont): | ||
| self.setFont(font) | ||
| self.header().setFont(font) |
There was a problem hiding this comment.
The update_font method in CapaExplorerQtreeView is missing a docstring. Adding one would improve code clarity and maintain consistency with other components in this file.
| def update_font(self, font: QtGui.QFont): | |
| self.setFont(font) | |
| self.header().setFont(font) | |
| def update_font(self, font: QtGui.QFont): | |
| """apply a new font to the tree view and its header""" | |
| self.setFont(font) | |
| self.header().setFont(font) |
github-actions
bot
dismissed
their stale review
CHANGELOG updated or no update needed, thanks! 😄
vee1e
changed the title
vee1e
changed the title
1 participant
Closes #2570
Summary
FixedFontso high‑DPI displays benefit from Qt’s DPI-aware sizingQFontDialog, persists the user’s choice inida_settings, and propagates it to the tree/editor/preview viewsupdate_font()hooks on the model/view components so a new font is immediately restyled without restarting IDA, while also keeping the dynamic scaling fallbackChecklist
Visual Output
MacOS
out.mp4
Linux
There's still bugs, a few nasty crashes and I haven't verified it on Linux/Windows so keeping this as draft for now.Disclaimer: Codex 5.1 mini was used to research on Qt documentation and parse the large crashdump report.