PR3: Display changelog/announcement page in SLEAP GUI #1556
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## shrivaths/changelog-announcement-2 #1556 +/- ##
======================================================================
+ Coverage 73.39% 73.46% +0.06%
======================================================================
Files 134 135 +1
Lines 24069 24139 +70
======================================================================
+ Hits 17666 17734 +68
- Misses 6403 6405 +2 ☔ View full report in Codecov by Sentry. |
|
CodeRabbit review skipped By default, CodeRabbit only reviews PRs on the default branch. If you wish to have PRs reviewed on additional branches, you can configure that under the repository settings in the UI. |
…gelog-announcement-3
…gelog-announcement-3
…gelog-announcement-3
WalkthroughThe SLEAP GUI has introduced a new feature to keep users informed of updates through a bulletin dialog. This feature includes the creation of a Changes
Related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- docs/bulletin.json
- tests/data/announcement_checker_bulletin/test_bulletin.json
Files selected for processing (5)
- sleap/gui/app.py (4 hunks)
- sleap/gui/dialogs/bulletin.py (1 hunks)
- sleap/gui/dialogs/bulletin/markdown.css (1 hunks)
- sleap/gui/dialogs/bulletin/markdown.html (1 hunks)
- sleap/gui/dialogs/bulletin/marked.js (1 hunks)
Files not summarized due to errors (1)
- sleap/gui/dialogs/bulletin/marked.js: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- sleap/gui/dialogs/bulletin/marked.js (Error: diff too large)
Files skipped from review due to trivial changes (1)
- sleap/gui/dialogs/bulletin/markdown.css
Additional comments: 5
sleap/gui/dialogs/bulletin/markdown.html (1)
- 1-30: The HTML file seems to be well-structured and the scripts are correctly placed. The use of the
marked.jslibrary for parsing Markdown and theqwebchannel.jslibrary for establishing a web channel is appropriate. TheupdateTextfunction correctly updates the content of the placeholder element with the parsed Markdown text. The web channel setup and connection of thetextChangedsignal to theupdateTextfunction are also correctly implemented.However, ensure that the paths to the
marked.jsandqwebchannel.jslibraries are correct and that these libraries are included in your project dependencies. Also, ensure that themarkdown.cssfile is correctly linked and that it exists in your project.sleap/gui/dialogs/bulletin.py (1)
- 1-32: The
BulletinDialogclass is well-structured and follows the Qt property and signal-slot mechanism. Thetextproperty is correctly defined with getter and setter methods, and a signaltextChangedis emitted whenever the text changes. This is a good practice in Qt for creating reactive properties.sleap/gui/app.py (3)
56-72: The imports are well organized and follow the PEP 8 style guide. The new imports are necessary for the new feature.
166-180: The code checks if there are any saved preferences for the "announcement last seen date" and "announcement". If not, it sets default values. This is a good practice as it ensures that the variables are always initialized.
191-193: The
AnnouncementCheckeris initialized and thenew_announcement_availablevariable is set. This is a good practice as it checks for new announcements during initialization.
sleap/gui/dialogs/bulletin.py
Outdated
| class BulletinDialog(QObject): | ||
| textChanged = Signal(str) | ||
|
|
||
| def __init__(self, parent=None): | ||
| super().__init__(parent) | ||
| self.m_text = "" | ||
|
|
||
| def get_text(self): | ||
| return self.m_text | ||
|
|
||
| def set_text(self, text): | ||
| if self.m_text == text: | ||
| return | ||
| self.m_text = text | ||
| self.textChanged.emit(self.m_text) | ||
|
|
||
| text = Property(str, fget=get_text, fset=set_text, notify=textChanged) |
There was a problem hiding this comment.
The BulletinDialog class currently only has a text property. If there are more properties or methods that are expected to be added in the future, consider adding a docstring to the class and its methods to explain their purpose and usage. This will improve the maintainability of the code.
| def __init__(self, parent=None): | ||
| super().__init__(parent) | ||
| self.m_text = "" | ||
|
|
There was a problem hiding this comment.
The __init__ method initializes m_text as an empty string. If there's a possibility that text could be passed during initialization, consider adding it as an optional argument to __init__. This would allow for more flexible usage of the class.
- def __init__(self, parent=None):
+ def __init__(self, parent=None, text=""):
super().__init__(parent)
- self.m_text = ""
+ self.m_text = textCommitable suggestion (Beta)
| def __init__(self, parent=None): | |
| super().__init__(parent) | |
| self.m_text = "" | |
| def __init__(self, parent=None, text=""): | |
| super().__init__(parent) | |
| self.m_text = text |
sleap/gui/app.py
Outdated
| # Display announcement bulletin popup | ||
| if self.new_announcement_available: | ||
| self.display_bulletin = BulletinDialog() | ||
| self.bulletin_dialog() | ||
|
|
||
| def bulletin_dialog(self): | ||
| """Displays bulletin dialog is new announcement is available.""" | ||
| announcement = self.announcement_checker.get_latest_announcement() | ||
|
|
||
| if announcement: | ||
| title, date, content = announcement | ||
| bulletin_markdown = "\n".join(content.split("\n")) | ||
|
|
||
| channel = QWebChannel() | ||
| channel.registerObject("content", self.display_bulletin) | ||
|
|
||
| self.display_bulletin.set_text(bulletin_markdown) | ||
|
|
||
| view = QWebEngineView() | ||
| view.page().setWebChannel(channel) | ||
| base_path = os.path.dirname(os.path.abspath(os.path.join(__file__))) | ||
| filepath = os.path.join( | ||
| base_path, "..", "gui", "dialogs", "bulletin", "markdown.html" | ||
| ) | ||
| url = QUrl.fromLocalFile(filepath) | ||
| view.load(url) | ||
| view.resize(720, 540) | ||
| view.show() | ||
| QApplication.exec_() | ||
|
|
There was a problem hiding this comment.
The bulletin_dialog method is well implemented. It checks if there is a new announcement and if so, it displays the bulletin dialog. The use of QWebChannel and QWebEngineView allows for the dynamic updating of the announcement content in the GUI. However, the bulletin_dialog method could be refactored to improve readability and maintainability. Consider breaking it down into smaller methods, each with a single responsibility. For example, one method could handle the creation of the QWebChannel and QWebEngineView, another could handle the loading of the URL, and another could handle the display of the view.
def bulletin_dialog(self):
"""Displays bulletin dialog is new announcement is available."""
announcement = self.announcement_checker.get_latest_announcement()
if announcement:
title, date, content = announcement
bulletin_markdown = "\n".join(content.split("\n"))
channel, view = self.create_web_channel_and_view(bulletin_markdown)
self.load_url(view)
self.display_view(view)
def create_web_channel_and_view(self, bulletin_markdown):
channel = QWebChannel()
channel.registerObject("content", self.display_bulletin)
self.display_bulletin.set_text(bulletin_markdown)
view = QWebEngineView()
view.page().setWebChannel(channel)
return channel, view
def load_url(self, view):
base_path = os.path.dirname(os.path.abspath(os.path.join(__file__)))
filepath = os.path.join(
base_path, "..", "gui", "dialogs", "bulletin", "markdown.html"
)
url = QUrl.fromLocalFile(filepath)
view.load(url)
def display_view(self, view):
view.resize(720, 540)
view.show()
QApplication.exec_()
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/app.py (4 hunks)
- sleap/gui/dialogs/bulletin.py (1 hunks)
Additional comments: 5
sleap/gui/app.py (5)
56-56: The import of
datefromdatetimeis new and should be used within theMainWindowclass to handle dates related to announcements. Ensure that this import is necessary and used appropriately.167-179: The initialization of
announcement last seen dateandannouncementis updated to use theprefsdictionary. This is a good use of the application's preferences system to store and retrieve persistent data. However, ensure that theprefsdictionary is properly updated elsewhere in the code when these values change, such as after an announcement has been seen.192-196: The initialization of
announcement_checkerand the check for new announcements are correctly placed in the__init__method. This ensures that the check is performed when theMainWindowis instantiated. However, it's important to ensure thatannouncement_checkerdoes not perform any blocking I/O operations on the main thread, which could cause the GUI to freeze. If it does, consider moving the check to a background thread.215-216: The call to
bulletin_dialogwithin the__init__method is conditionally executed if a new announcement is available. This is a good practice as it avoids unnecessary function calls. However, ensure that thebulletin_dialogmethod handles all possible exceptions, as any unhandled exceptions here could prevent the main window from initializing properly.218-231: The new
bulletin_dialogmethod is responsible for displaying the bulletin dialog. It retrieves the latest announcement and creates aBulletinDialogandBulletinWorkerto display the announcement. Ensure that theBulletinWorkeris properly managed and does not lead to any resource leaks. Additionally, verify that theBulletinDialogis modal or non-modal as intended and that it behaves correctly in the context of the application (e.g., user can close it, it doesn't block other interactions if non-modal, etc.).
sleap/gui/dialogs/bulletin.py
Outdated
| class BulletinWorker(QThread): | ||
| text_updated = Signal(str) | ||
|
|
||
| def __init__(self, content, parent=None): | ||
| super(BulletinWorker, self).__init__(parent) | ||
| self.content = content | ||
|
|
||
| def run(self): | ||
| self.text_updated.emit(self.content) | ||
|
|
There was a problem hiding this comment.
The BulletinWorker class is designed to emit a signal with the content when run. It is crucial to ensure that the content variable is properly sanitized or validated before being used to prevent any potential security risks, especially since the content will be rendered in a web view which could be a vector for XSS attacks if the content includes HTML or JavaScript code. Additionally, consider handling any exceptions that might occur during the thread execution to prevent the application from crashing.
sleap/gui/dialogs/bulletin.py
Outdated
| class BulletinDialog(QDialog): | ||
| def __init__(self, parent=None): | ||
| super(BulletinDialog, self).__init__(parent) | ||
|
|
||
| self.label = QLabel() | ||
| layout = QVBoxLayout() | ||
| layout.addWidget(self.label) | ||
| self.setLayout(layout) | ||
|
|
||
| @Slot(str) | ||
| def updateText(self, text): | ||
| self.label.setText(text) |
There was a problem hiding this comment.
The BulletinDialog class is well-structured and follows the Qt conventions for signal-slot mechanisms. However, it's important to ensure that the updateText slot is only connected to trusted signals, as it directly updates the UI with the provided text. If the text comes from an untrusted source, it could pose a security risk. Additionally, consider adding error handling for the UI components in case of unexpected input or issues during the update.
sleap/gui/app.py
Outdated
| # Display announcement bulletin popup | ||
| if self.new_announcement_available: | ||
| self.bulletin_dialog() | ||
|
|
||
| def bulletin_dialog(self): | ||
| """Displays bulletin dialog is new announcement is available.""" | ||
| announcement = self.announcement_checker.get_latest_announcement() | ||
|
|
||
| if announcement: | ||
| title, date, content = announcement | ||
| bulletin_markdown = "\n".join(content.split("\n")) | ||
|
|
||
| # Create the pop-up dialog | ||
| popup_dialog = BulletinDialog(self) | ||
|
|
||
| # Set the dialog as a top-level window with the Qt.WindowStaysOnTopHint flag | ||
| popup_dialog.setWindowFlags( | ||
| popup_dialog.windowFlags() | Qt.WindowStaysOnTopHint | ||
| ) | ||
|
|
||
| # Show the dialog | ||
| popup_dialog.show() | ||
|
|
||
| # Create a worker thread to update the text in the dialog | ||
| popup_worker = BulletinWorker(bulletin_markdown) | ||
| popup_worker.text_updated.connect(popup_dialog.updateText) | ||
| popup_worker.start() | ||
|
|
||
| # Save the dialog and worker so we can close them later | ||
| # Otherwise get "QThread: Destroyed while thread is still running" | ||
| self._child_windows["bulletin_dialog"] = popup_dialog # Not really needed | ||
| self._child_windows["bulletin_worker"] = popup_worker # Needed! |
There was a problem hiding this comment.
The bulletin_dialog method is well-structured and creates a dialog and worker thread to display announcements. However, there are a few points to consider:
- The dialog is set to always stay on top, which might be intrusive for the user. Consider providing an option to disable this behavior.
- The worker thread is started without checking if it's already running or not. If
bulletin_dialogis called multiple times, it could spawn multiple threads. It's important to manage the lifecycle of these threads to avoid potential resource leaks. - The dialog and worker are stored in
_child_windows, but the comment indicates that storing the dialog might not be necessary. Verify if this is the case and remove it if it's not needed. - The
bulletin_markdownvariable seems redundant as it just joins the content split by newlines, which doesn't change the content. This could be simplified.
- bulletin_markdown = "\n".join(content.split("\n"))
+ bulletin_markdown = contentCommitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| # Display announcement bulletin popup | |
| if self.new_announcement_available: | |
| self.bulletin_dialog() | |
| def bulletin_dialog(self): | |
| """Displays bulletin dialog is new announcement is available.""" | |
| announcement = self.announcement_checker.get_latest_announcement() | |
| if announcement: | |
| title, date, content = announcement | |
| bulletin_markdown = "\n".join(content.split("\n")) | |
| # Create the pop-up dialog | |
| popup_dialog = BulletinDialog(self) | |
| # Set the dialog as a top-level window with the Qt.WindowStaysOnTopHint flag | |
| popup_dialog.setWindowFlags( | |
| popup_dialog.windowFlags() | Qt.WindowStaysOnTopHint | |
| ) | |
| # Show the dialog | |
| popup_dialog.show() | |
| # Create a worker thread to update the text in the dialog | |
| popup_worker = BulletinWorker(bulletin_markdown) | |
| popup_worker.text_updated.connect(popup_dialog.updateText) | |
| popup_worker.start() | |
| # Save the dialog and worker so we can close them later | |
| # Otherwise get "QThread: Destroyed while thread is still running" | |
| self._child_windows["bulletin_dialog"] = popup_dialog # Not really needed | |
| self._child_windows["bulletin_worker"] = popup_worker # Needed! | |
| # Display announcement bulletin popup | |
| if self.new_announcement_available: | |
| self.bulletin_dialog() | |
| def bulletin_dialog(self): | |
| """Displays bulletin dialog is new announcement is available.""" | |
| announcement = self.announcement_checker.get_latest_announcement() | |
| if announcement: | |
| title, date, content = announcement | |
| bulletin_markdown = content | |
| # Create the pop-up dialog | |
| popup_dialog = BulletinDialog(self) | |
| # Set the dialog as a top-level window with the Qt.WindowStaysOnTopHint flag | |
| popup_dialog.setWindowFlags( | |
| popup_dialog.windowFlags() | Qt.WindowStaysOnTopHint | |
| ) | |
| # Show the dialog | |
| popup_dialog.show() | |
| # Create a worker thread to update the text in the dialog | |
| popup_worker = BulletinWorker(bulletin_markdown) | |
| popup_worker.text_updated.connect(popup_dialog.updateText) | |
| popup_worker.start() | |
| # Save the dialog and worker so we can close them later | |
| # Otherwise get "QThread: Destroyed while thread is still running" | |
| self._child_windows["bulletin_worker"] = popup_worker # Needed! |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/app.py (5 hunks)
- sleap/gui/dialogs/bulletin.py (1 hunks)
Additional comments: 5
sleap/gui/app.py (4)
56-56: The import of
datefromdatetimeis correctly added and used for initializing theannouncement last seen date.215-228: The
bulletin_dialogmethod is correctly implemented to display a bulletin dialog if a new announcement is available.189-190: The
announcement_checkerandnew_announcement_availableattributes are correctly initialized in the__init__method.212-213: A conditional check is correctly added to display the bulletin dialog if a new announcement is available.
sleap/gui/dialogs/bulletin.py (1)
- 13-57: The implementation of the
BulletinWorkerandDocumentclasses appears to be correct and well-structured. TheBulletinWorkerclass properly initializes with content and sets up the web view, while theDocumentclass manages the text content and emits a signal when the text is changed. The previous comment about adding an optionaltextargument to theDocumentclass__init__method is still valid and should be considered for flexibility.
sleap/gui/app.py
Outdated
| if prefs["announcement"]: | ||
| self.state["announcement"] = prefs["announcement"] | ||
| else: | ||
| self.state["announcement"] = "No data to display" |
There was a problem hiding this comment.
The default value for announcement should be None or an empty string to clearly indicate the absence of an announcement.
- self.state["announcement"] = "No data to display"
+ self.state["announcement"] = NoneCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| self.state["announcement"] = "No data to display" | |
| self.state["announcement"] = None |
sleap/gui/app.py
Outdated
| self.announcement_checker = AnnouncementChecker(state=self.state) | ||
| self.new_announcement_available = self.announcement_checker.new_announcement | ||
|
|
||
| if self.state["share usage data"]: | ||
| ping_analytics() |
There was a problem hiding this comment.
The ping_analytics function should be wrapped in a conditional check to respect user preferences.
- ping_analytics()
+ if self.state["share usage data"]:
+ ping_analytics()Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ping_analytics() | |
| if self.state["share usage data"]: | |
| ping_analytics() |
…gelog-announcement-3
…gelog-announcement-3
…gelog-announcement-3
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- docs/bulletin.json
Files selected for processing (3)
- sleap/gui/app.py (5 hunks)
- sleap/gui/dialogs/bulletin/markdown.css (1 hunks)
- tests/gui/test_app.py (1 hunks)
Files skipped from review due to trivial changes (1)
- sleap/gui/dialogs/bulletin/markdown.css
Additional comments: 5
sleap/gui/app.py (5)
53-59: The import of
datetime.dateis correctly added as per the PR objectives and summary.66-72: The import of
BulletinWorkeris correctly added as per the PR objectives and summary.160-166: The assignment of
announcement last seen dateandannouncementtoself.statehas been moved within the__init__method as mentioned in the summary.196-202: The conditional block to check for a new announcement and display the bulletin dialog accordingly is present as mentioned in the summary.
204-211: The method
bulletin_dialogis added to the class as mentioned in the summary.
tests/gui/test_app.py
Outdated
| bulletin_dialog = app._child_windows.get("bulletin_worker", False) | ||
| if app.announcement_checker.new_announcement_available(): | ||
| print("bulletin available") | ||
| assert isinstance(bulletin_dialog, BulletinWorker) | ||
| else: | ||
| print("no bulletin") | ||
| assert bulletin_dialog == False |
There was a problem hiding this comment.
The test checks for the existence of a BulletinWorker in app._child_windows which is incorrect. It should check for an instance of BulletinDialog. Also, the assertion should use is instead of == for comparing with False.
- bulletin_dialog = app._child_windows.get("bulletin_worker", False)
+ bulletin_dialog = app._child_windows.get("bulletin_dialog", None)
- assert bulletin_dialog == False
+ assert bulletin_dialog is FalseCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| bulletin_dialog = app._child_windows.get("bulletin_worker", False) | |
| if app.announcement_checker.new_announcement_available(): | |
| print("bulletin available") | |
| assert isinstance(bulletin_dialog, BulletinWorker) | |
| else: | |
| print("no bulletin") | |
| assert bulletin_dialog == False | |
| bulletin_dialog = app._child_windows.get("bulletin_dialog", None) | |
| if app.announcement_checker.new_announcement_available(): | |
| print("bulletin available") | |
| assert isinstance(bulletin_dialog, BulletinWorker) | |
| else: | |
| print("no bulletin") | |
| assert bulletin_dialog is False |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/app.py (5 hunks)
- tests/gui/test_app.py (1 hunks)
Additional comments: 9
sleap/gui/app.py (7)
- 53-59: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [53-69]
The import statements for
dateandBulletinWorkerare correctly added to support the new feature implementation.
207-215: The addition of the
bulletin_dialogmethod aligns with the PR objectives to display a bulletin popup when there is a new announcement.196-218: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [160-206]
The changes to the
__init__method for initializing the announcement checker and displaying the bulletin dialog are consistent with the PR objectives.
- 196-218: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [53-215]
The summary mentions GUI enhancements using
QWebChannelandQWebEngineView, and modifications tomarkdown.css, but these changes are not visible in the provided hunks. Please ensure that these changes are included in the PR if they are part of the new feature.
- 196-218: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [53-215]
The summary mentions the introduction of a
BulletinWorkerclass for asynchronous content management, but the implementation details are not visible in the provided hunks. Please verify that the implementation ofBulletinWorkeris included in the PR.
- 196-218: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [53-215]
The summary mentions updates to the test suite in
test_app.pyto accommodate the new feature, but no test code is provided in the hunks. Please ensure that the test suite is updated accordingly.
- 196-218: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [53-215]
The summary mentions new dependencies on external JavaScript libraries, but these are not visible in the provided hunks. Please verify that the dependencies are correctly added to the project.
tests/gui/test_app.py (2)
9-19: The test logic for the bulletin dialog correctly checks the presence or absence of the bulletin based on the
new_announcement_availableflag. This aligns with the PR's objective to introduce a changelog or announcement page.21-23: The rest of the test code remains unchanged and is not related to the bulletin feature. It appears to be testing other functionalities of the application.
…gelog-announcement-3
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- docs/bulletin.json
Files selected for processing (5)
- sleap/gui/app.py (5 hunks)
- sleap/gui/dialogs/bulletin.py (1 hunks)
- sleap/gui/dialogs/bulletin/markdown.css (1 hunks)
- sleap/gui/dialogs/bulletin/markdown.html (1 hunks)
- sleap/gui/dialogs/bulletin/marked.js (1 hunks)
Files not summarized due to errors (1)
- sleap/gui/dialogs/bulletin/marked.js: Error: Message exceeds token limit
Additional comments: 12
sleap/gui/dialogs/bulletin/markdown.html (2)
1-69: The HTML structure and JavaScript code appear to be correctly implemented for the intended functionality of displaying and navigating markdown content. Ensure that the
marked.jslibrary is included in the project or accessible at runtime, as it is essential for rendering markdown content.35-66: The logic for creating a navigation bar based on
h3tags is implemented correctly. However, consider adding comments to explain the purpose of the code and the reason for choosingh3tags specifically, as this will improve code maintainability.sleap/gui/dialogs/bulletin.py (2)
13-53: The
BulletinWorkerclass is well implemented with appropriate window flags, geometry settings, and centering logic. The use ofQWebChannelandQWebEngineViewis correct for the intended functionality. Ensure that themarkdown.htmlfile is located correctly relative to thebulletin.pyfile for theQUrl.fromLocalFilemethod to work as expected.56-72: The
Documentclass correctly implements a Qt signaltextChangedand a propertytextto handle changes to the bulletin text. This is a good use of Qt's property system and signals/slots mechanism.sleap/gui/dialogs/bulletin/markdown.css (2)
1-344: The CSS styles are well-defined and follow best practices for readability and maintainability. The use of relative units, such as em for padding and margins, and the use of hexadecimal color codes with alpha transparency are modern CSS practices. Ensure that the CSS file is included in the project and is accessible at runtime.
135-135: Verify that the code below the copyright comment is allowed to be used within this project and that it complies with any licensing requirements. This is important for legal compliance and to avoid potential copyright infringement.
sleap/gui/app.py (6)
56-56: The import of
datefrom thedatetimemodule is added. Ensure that this import is used within the file and is not extraneous.69-69: The import of
BulletinWorkeris added. Verify that this class is used in the file and that the import is necessary.163-166: The handling of the
no_usage_dataflag is correct and follows the existing pattern for setting state variables based on initialization parameters.175-176: The initialization of
announcement last seen dateandannouncementusing preferences is added. Ensure that these preferences are properly set and saved elsewhere in the application to maintain state consistency.199-206: The logic to check for new announcements and display the bulletin dialog if available is added. This is a key part of the new feature implementation. Ensure that the
new_announcement_availablemethod ofAnnouncementCheckeris correctly implemented and that theupdate_announcementandbulletin_dialogmethods are properly defined and handle all edge cases.Verification successful
I will refine the ast-grep patterns to verify the presence and implementation of the
new_announcement_available,update_announcement, andbulletin_dialogmethods in theAnnouncementCheckerandMainWindowclasses, respectively.
I will now use ripgrep (rg) to search for the method names directly in the codebase to verify the existence and implementation of the
new_announcement_available,update_announcement, andbulletin_dialogmethods.
The shell script results confirm the existence of the
new_announcement_available,update_announcement, andbulletin_dialogmethods within the codebase. Specifically:
- The
new_announcement_availablemethod is found insleap/gui/web.py.- The
update_announcementmethod is also found insleap/gui/web.py.- The
bulletin_dialogmethod is found insleap/gui/app.py.These findings align with the expectations set by the review comment, confirming that the methods are present and suggesting that they are likely implemented as intended.
* 207-217: The `bulletin_dialog` method is added to display the bulletin dialog. This method initializes a `BulletinWorker` with the announcement text and shows the bulletin. Ensure that the `BulletinWorker` class and its `show_bulletin` method are correctly implemented and that the `announcement` state variable contains the expected text format.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the AnnouncementChecker class has the new_announcement_available method. ast-grep --lang python --pattern $'class AnnouncementChecker { $$$ new_announcement_available($_) { $$$ } $$$ }' # Verify that the update_announcement method is properly implemented. ast-grep --lang python --pattern $'class AnnouncementChecker { $$$ update_announcement($_) { $$$ } $$$ }' # Verify that the bulletin_dialog method is properly implemented within MainWindow. ast-grep --lang python --pattern $'class MainWindow { $$$ bulletin_dialog($_) { $$$ } $$$ }'Length of output: 1349
Script:
#!/bin/bash # Refine the pattern to check for the presence of the new_announcement_available method in the AnnouncementChecker class. ast-grep --lang python --pattern $'class AnnouncementChecker { def new_announcement_available($_) { $$$ } }' # Refine the pattern to check for the presence of the update_announcement method in the AnnouncementChecker class. ast-grep --lang python --pattern $'class AnnouncementChecker { def update_announcement($_) { $$$ } }' # Refine the pattern to check for the presence of the bulletin_dialog method in the MainWindow class. ast-grep --lang python --pattern $'class MainWindow { def bulletin_dialog($_) { $$$ } }'Length of output: 1337
Script:
#!/bin/bash # Search for the new_announcement_available method in the AnnouncementChecker class. rg "def new_announcement_available" # Search for the update_announcement method in the AnnouncementChecker class. rg "def update_announcement" # Search for the bulletin_dialog method in the MainWindow class. rg "def bulletin_dialog"Length of output: 262
Verification successful
The shell script execution confirms the existence of the
BulletinWorkerclass and theshow_bulletinmethod within thesleap/gui/dialogs/bulletin.pyfile. This information verifies that theBulletinWorkerclass and itsshow_bulletinmethod are correctly implemented as per the review comment.
- The
BulletinWorkerclass is defined insleap/gui/dialogs/bulletin.py.- The
show_bulletinmethod is a member of theBulletinWorkerclass in the same file.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the BulletinWorker class and show_bulletin method are correctly implemented. ast-grep --lang python --pattern $'class BulletinWorker { $$$ show_bulletin($_) { $$$ } $$$ }'Length of output: 440
Script:
#!/bin/bash # Refine the search for the BulletinWorker class and the show_bulletin method. ast-grep --lang python --pattern $'class BulletinWorker { $$$ show_bulletin() { $$$ } $$$ }'Length of output: 438
Script:
#!/bin/bash # Search for the BulletinWorker class definition and the show_bulletin method in the codebase. rg "class BulletinWorker" rg "def show_bulletin"Length of output: 186
| // Wait till the document is loaded to add navbar | ||
| setTimeout(function() { | ||
| executeSecondScript(); | ||
| }, 100); |
There was a problem hiding this comment.
Using setTimeout to wait for the document to load before executing executeSecondScript may not be reliable. Consider using the DOMContentLoaded event to ensure that the script is executed after the document is fully loaded.
document.addEventListener('DOMContentLoaded', (event) => {
executeSecondScript();
});| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>What's New?</title> | ||
| <link rel="stylesheet" type="text/css" href="markdown.css"> | ||
| <script src="marked.js"></script> | ||
| <script src="qrc:/qtwebchannel/qwebchannel.js"></script> | ||
| </head> | ||
| <body> | ||
| <div id="placeholder"></div> | ||
| <div id="navbar"></div> | ||
| <script> | ||
| 'use strict'; | ||
|
|
||
| var placeholder = document.getElementById('placeholder'); | ||
|
|
||
| var updateText = function(text) { | ||
| placeholder.innerHTML = marked(text); | ||
| // Wait till the document is loaded to add navbar | ||
| setTimeout(function() { | ||
| executeSecondScript(); | ||
| }, 100); | ||
| } | ||
|
|
||
| new QWebChannel(qt.webChannelTransport, | ||
| function(channel) { | ||
| var content = channel.objects.content; | ||
| updateText(content.text); | ||
| content.textChanged.connect(updateText); | ||
| } | ||
| ); | ||
|
|
||
| function executeSecondScript() { | ||
| const hTags = document.querySelectorAll('h3'); | ||
| const navbar = document.getElementById('navbar'); | ||
| const ul = document.createElement('ul'); | ||
| var uniqueIdCounter = 1; | ||
|
|
||
| hTags.forEach((h, index) => { | ||
| const li = document.createElement('li'); | ||
| const link = document.createElement('a'); | ||
| // Assign a unique ID if one doesn't exist | ||
| if (!h.id) { | ||
| h.id = `generated-id-${uniqueIdCounter}`; | ||
| uniqueIdCounter++; | ||
| } | ||
| link.href = '#' + h.id; // Create anchor links based on h2 IDs | ||
| link.textContent = h.textContent; | ||
| link.addEventListener('click', function(event) { | ||
| event.preventDefault(); | ||
| scrollToSection(index); | ||
| }); | ||
|
|
||
| li.appendChild(link); | ||
| ul.appendChild(li); | ||
| }); | ||
|
|
||
| navbar.appendChild(ul); | ||
| document.body.insertBefore(navbar, document.body.firstChild) | ||
| }; | ||
| function scrollToSection(index) { | ||
| var targetSection = document.querySelectorAll('h3')[index]; | ||
| targetSection.scrollIntoView({ behavior: 'smooth' }); | ||
| } | ||
| </script> | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [17-70]
Consider replacing var with let or const for variable declarations to follow modern JavaScript best practices and to avoid potential issues with variable hoisting and scoping.
- var placeholder = document.getElementById('placeholder');
+ const placeholder = document.getElementById('placeholder');
// ... and so on for other variables
sleap/gui/dialogs/bulletin.py
Outdated
| # class MyWebEngineView(QWebEngineView): | ||
| # def createWindow(self, type): | ||
| # new_view = MyWebEngineView(self) | ||
| # new_view.show() | ||
| # return new_view | ||
|
|
||
| # def acceptNavigationRequest(self, url, navigation_type, is_main_frame): | ||
| # if navigation_type == QWebEnginePage.NavigationTypeLinkClicked: | ||
| # # Emit the linkClicked signal when a link is clicked | ||
| # self.page().mainFrame().javaScriptWindowObjectCleared.connect( | ||
| # lambda: self.page().mainFrame().addToJavaScriptWindowObject("linkHandler", self) | ||
| # ) | ||
| # self.page().runJavaScript("linkHandler.linkClicked('%s');" % url.toString()) | ||
| # return False | ||
| # return super().acceptNavigationRequest(url, navigation_type, is_main_frame) |
There was a problem hiding this comment.
Commented-out code, such as the MyWebEngineView class, should be removed to keep the codebase clean and maintainable unless there is a specific reason to keep it for future reference.
- # class MyWebEngineView(QWebEngineView):
- # def createWindow(self, type):
- # ...
- # return new_view
- # ...Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| # class MyWebEngineView(QWebEngineView): | |
| # def createWindow(self, type): | |
| # new_view = MyWebEngineView(self) | |
| # new_view.show() | |
| # return new_view | |
| # def acceptNavigationRequest(self, url, navigation_type, is_main_frame): | |
| # if navigation_type == QWebEnginePage.NavigationTypeLinkClicked: | |
| # # Emit the linkClicked signal when a link is clicked | |
| # self.page().mainFrame().javaScriptWindowObjectCleared.connect( | |
| # lambda: self.page().mainFrame().addToJavaScriptWindowObject("linkHandler", self) | |
| # ) | |
| # self.page().runJavaScript("linkHandler.linkClicked('%s');" % url.toString()) | |
| # return False | |
| # return super().acceptNavigationRequest(url, navigation_type, is_main_frame) |
| return done(); | ||
| } | ||
|
|
||
| delete opt.highlight; | ||
|
|
||
| if (!pending) return done(); | ||
|
|
||
| for (; i < tokens.length; i++) { | ||
| (function (token) { | ||
| if (token.type !== 'code') { | ||
| return --pending || done(); | ||
| } | ||
| return highlight(token.text, token.lang, function (err, code) { | ||
| if (err) return done(err); | ||
| if (code == null || code === token.text) { | ||
| return --pending || done(); | ||
| } | ||
| token.text = code; | ||
| token.escaped = true; | ||
| --pending || done(); | ||
| }); | ||
| })(tokens[i]); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
| try { | ||
| if (opt) opt = merge({}, marked.defaults, opt); | ||
| return Parser.parse(Lexer.lex(src, opt), opt); | ||
| } catch (e) { | ||
| e.message += '\nPlease report this to https://github.com/markedjs/marked.'; | ||
| if ((opt || marked.defaults).silent) { | ||
| return '<p>An error occurred:</p><pre>' | ||
| + escape(e.message + '', true) | ||
| + '</pre>'; | ||
| } | ||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Options | ||
| */ | ||
|
|
||
| marked.options = | ||
| marked.setOptions = function (opt) { | ||
| merge(marked.defaults, opt); | ||
| return marked; | ||
| }; | ||
|
|
||
| marked.getDefaults = function () { | ||
| return { | ||
| baseUrl: null, | ||
| breaks: false, | ||
| gfm: true, | ||
| headerIds: true, | ||
| headerPrefix: '', | ||
| highlight: null, | ||
| langPrefix: 'language-', | ||
| mangle: true, | ||
| pedantic: false, | ||
| renderer: new Renderer(), | ||
| sanitize: false, | ||
| sanitizer: null, | ||
| silent: false, | ||
| smartLists: false, | ||
| smartypants: false, | ||
| tables: true, | ||
| xhtml: false | ||
| }; | ||
| } | ||
|
|
||
| marked.defaults = marked.getDefaults(); | ||
|
|
||
| /** | ||
| * Expose | ||
| */ | ||
|
|
||
| marked.Parser = Parser; | ||
| marked.parser = Parser.parse; | ||
|
|
||
| marked.Renderer = Renderer; | ||
| marked.TextRenderer = TextRenderer; | ||
|
|
||
| marked.Lexer = Lexer; | ||
| marked.lexer = Lexer.lex; | ||
|
|
||
| marked.InlineLexer = InlineLexer; | ||
| marked.inlineLexer = InlineLexer.output; | ||
|
|
||
| marked.parse = marked; | ||
|
|
||
| if (typeof module !== 'undefined' && typeof exports === 'object') { | ||
| module.exports = marked; | ||
| } else if (typeof define === 'function' && define.amd) { | ||
| define(function () { return marked; }); | ||
| } else { | ||
| root.marked = marked; | ||
| } | ||
| })(this || (typeof window !== 'undefined' ? window : global)); |
There was a problem hiding this comment.
The shell script output indicates that marked.js is indeed present in the sleap/gui/dialogs/bulletin directory and that there has been a recent change to the file. The change is a minor edit to the formatting of the HTML output for links, specifically adding a space before target="_blank".
Given this information, it can be confirmed that:
- The
marked.jsfile is included in the project. - A minor modification has been made to the file.
Analysis chain
Please confirm that the inclusion of the entire marked.js library is intentional and verify that it is correctly integrated into the SLEAP project, considering the project's existing markdown parsing and rendering strategies.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that marked.js is not already included or available in the project's dependencies
fd 'marked.js'
# Check for any custom modifications in the marked.js file
git diff HEAD~1 -- sleap/gui/dialogs/bulletin/marked.js
Length of output: 555
…gelog-announcement-3
…gelog-announcement-3
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- sleap/gui/app.py (5 hunks)
- sleap/gui/dialogs/bulletin.py (1 hunks)
- sleap/gui/dialogs/bulletin/markdown.css (1 hunks)
- sleap/gui/dialogs/bulletin/markdown.html (1 hunks)
- sleap/gui/dialogs/bulletin/pico.min.css (1 hunks)
- tests/io/test_formats.py (1 hunks)
Files not summarized due to errors (1)
- sleap/gui/dialogs/bulletin/pico.min.css: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- tests/io/test_formats.py
Additional comments: 17
sleap/gui/dialogs/bulletin/markdown.css (4)
1-3: The base font family is set to Helvetica, Arial, sans-serif which is a good choice for cross-platform compatibility.
5-11: The color and font-style for headings are consistent and provide a visually distinct style for the bulletin content.
14-30: The
#containerand#navbarstyles are set up for a fixed sidebar layout. Ensure that this layout is responsive and works well with the rest of the GUI elements.42-47: The
#contentstyle has a left margin set to match the navbar width. This is a good practice to ensure the content does not overlap with the navbar.sleap/gui/dialogs/bulletin/markdown.html (3)
1-10: The HTML structure is well-formed with appropriate meta tags and title. External stylesheets and scripts are linked correctly.
13-16: The container div is set up to hold the content and navbar, which aligns with the CSS layout.
17-68: The script is using modern JavaScript practices such as 'use strict' and is structured to update the content and generate a navbar dynamically. Ensure that the
marked.jslibrary is included in the project and is up-to-date to prevent any security vulnerabilities.sleap/gui/dialogs/bulletin.py (4)
13-22: The
BulletinWorkerclass has been renamed to reflect its purpose more accurately, and the window flags are set to keep the bulletin on top.23-32: The
center_on_screenmethod correctly calculates the center position for the bulletin window. This is a user-friendly feature.34-53: The
show_bulletinmethod sets up theDocumentobject and the web channel, loads the HTML file, and shows the bulletin. The path to the HTML file is correctly constructed usingpathlib.56-72: The
Documentclass has been updated to include the optionaltextparameter in the__init__method as suggested in the previous review. This allows setting the initial text upon instantiation.sleap/gui/app.py (6)
56-56: The import of
datefrom thedatetimemodule is added as per the summary. This is a standard import and there are no issues with it.69-69: The import of
BulletinWorkeris added as per the summary. This is a standard import and there are no issues with it.163-163: The assignment of
self.state["clipboard_track"]andself.state["clipboard_instance"]toNoneis added. This seems to be a part of the state initialization. Ensure that these new state keys are used appropriately elsewhere in the application.Verification successful
The review comment is verified. The new state keys
clipboard_trackandclipboard_instanceare used appropriately within the application's logic insleap/gui/commands.pyand are included in the test cases intests/gui/test_commands.py.* 174-177: The initialization of `self.release_checker` and the assignment of `self.state["announcement last seen date"]` and `self.state["announcement"]` are added. Ensure that these changes are consistent with the application's logic for handling announcements and release checking.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that "clipboard_track" and "clipboard_instance" are used correctly. rg --type py 'state\["clipboard_track"\]' 'state\["clipboard_instance"\]'Length of output: 145
Script:
#!/bin/bash # Verify that "clipboard_track" and "clipboard_instance" are used correctly. # The previous command was incorrect, so we're using a corrected version to search for both keys. rg --type py 'state\["clipboard_(track|instance)"\]'Length of output: 1714
Verification successful
The search results indicate that the
state["announcement last seen date"]andstate["announcement"]are indeed used within the codebase. Specifically, they are referenced in thesleap/gui/web.py,sleap/gui/app.py, andtests/gui/test_web.pyfiles. The usage inapp.pyaligns with the initialization mentioned in the review comment, and the references inweb.pyandtest_web.pysuggest that these state keys are part of the logic for handling announcements and release checking.Based on the search results, it can be concluded that the initialization of
self.state["announcement last seen date"]andself.state["announcement"]insleap/gui/app.pyis consistent with their usage in the application's logic for handling announcements and release checking.* 199-206: The logic for checking if a new announcement is available and displaying the bulletin dialog is added. This is a new feature as per the summary. Ensure that the method `bulletin_dialog` is properly defined and that the logic for determining if a new announcement is available is correct.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that "announcement last seen date" and "announcement" are used correctly. rg --type py 'state\["announcement last seen date"\]' 'state\["announcement"\]'Length of output: 145
Script:
#!/bin/bash # Correcting the syntax and verifying the usage of "announcement last seen date" and "announcement" in the codebase. rg --type py 'state\["announcement last seen date"\]' rg --type py 'state\["announcement"\]'Length of output: 1099
Verification successful
The
bulletin_dialogmethod is defined insleap/gui/app.pyand the provided script output shows the method's implementation. The method initializes aBulletinWorkerwith the announcement text and the application instance (self) as arguments, then callsshow_bulletin()on thepopup_workerobject. It also saves thepopup_workertoself._child_windowswith the key"bulletin_worker".Based on the provided context and the output from the script, the
bulletin_dialogmethod appears to be properly defined and used within thesleap/gui/app.pyfile. The method's purpose is to display the bulletin dialog if a new announcement is available, which aligns with the PR objectives and the review comment.* 207-217: The new method `bulletin_dialog` is added for displaying a bulletin popup. The method initializes a `BulletinWorker` and shows the bulletin. It also saves the `popup_worker` to `self._child_windows`. Ensure that the `BulletinWorker` is properly implemented and that the `show_bulletin` method exists and functions as expected.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the bulletin_dialog method is properly defined and used. rg --type py 'bulletin_dialog'Length of output: 366
Script:
#!/bin/bash # Let's check the definition of the bulletin_dialog method in sleap/gui/app.py rg --type py -A 10 'def bulletin_dialog\(self\):' sleap/gui/app.pyLength of output: 518
Verification successful
The provided script output confirms the existence and implementation of the
BulletinWorkerclass and theshow_bulletinmethod within thesleap/gui/dialogs/bulletin.pyfile. TheBulletinWorkerclass is derived fromQtWidgets.QMainWindowand includes an__init__method that sets up the bulletin window with specific flags, title, and geometry. It also includes a method to center the window on the screen.The
show_bulletinmethod is part of theBulletinWorkerclass and is responsible for creating aDocumentobject, setting up aQWebChannel, and registering the document with the channel. It then sets the text of the document to the content passed during initialization and creates aQWebEngineViewto display the content.Based on the output, it appears that the
BulletinWorkerandshow_bulletinmethod are properly implemented as they are defined in the code and include logic for displaying the bulletin.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that BulletinWorker and show_bulletin are properly implemented. rg --type py 'BulletinWorker' rg --type py 'show_bulletin'Length of output: 613
Script:
#!/bin/bash # Check the implementation of BulletinWorker class and show_bulletin method in bulletin.py rg --type py -A 30 'class BulletinWorker' sleap/gui/dialogs/bulletin.py rg --type py -A 10 'def show_bulletin' sleap/gui/dialogs/bulletin.pyLength of output: 1568
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (5)
- tests/gui/learning/test_dialog.py (1 hunks)
- tests/gui/test_app.py (1 hunks)
- tests/gui/test_grid_system.py (1 hunks)
- tests/gui/widgets/test_docks.py (1 hunks)
- tests/io/test_formats.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/io/test_formats.py
Additional comments: 2
tests/gui/test_grid_system.py (1)
- 2-3: The addition of imports for
pytestandsysis correct and follows standard Python conventions.tests/gui/test_app.py (1)
- 22-27: The logic to check if the bulletin is shown or not appears to be correct. It uses the
new_announcement_availableflag to determine if the bulletin dialog should be an instance ofBulletinWorker. Ensure that thenew_announcement_availableflag is being set correctly elsewhere in the code.Verification successful
The verification process has confirmed that the
new_announcement_availableflag is set and used correctly within the codebase. TheAnnouncementCheckerclass inweb.pydefines the logic for checking if a new announcement is available, and this logic is utilized in theMainWindowclass constructor inapp.pyto set the flag. The test intest_app.pyappropriately checks the bulletin dialog based on this flag. The review comment is consistent with the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new_announcement_available flag is set and used correctly. rg --type python 'new_announcement_available'Length of output: 76
Script:
#!/bin/bash # Verify that the new_announcement_available flag is set and used correctly. rg 'new_announcement_available' --type pyLength of output: 620
Script:
#!/bin/bash # Verify the definition and setting of new_announcement_available in web.py cat sleap/gui/web.py # Verify the logic in app.py that sets the new_announcement_available flag cat sleap/gui/app.pyLength of output: 70337
tests/gui/test_grid_system.py
Outdated
| if sys.platform == "darwin": | ||
| pytestmark = pytest.mark.skip(reason="ImportError for MacOS") | ||
| pytest.skip(reason="Skipping tests and import on macOS", allow_module_level=True) |
There was a problem hiding this comment.
The conditional block to skip tests on MacOS due to QWebEngineView ImportError is implemented correctly. However, the TODO comment should be more actionable. Consider specifying what steps might be taken to resolve the ImportError or linking to an issue tracker for better follow-up.
tests/gui/widgets/test_docks.py
Outdated
| import sys | ||
|
|
||
| # Skip the test for MacOS due to QWebEngineView ImportError | ||
| # TODO: Need to resolve the ImportError | ||
| if sys.platform == "darwin": | ||
| pytestmark = pytest.mark.skip(reason="ImportError for MacOS") | ||
| pytest.skip(reason="Skipping tests and import on macOS", allow_module_level=True) |
There was a problem hiding this comment.
The conditional block to skip tests on MacOS due to QWebEngineView ImportError is implemented correctly. As previously mentioned, the TODO comment should be more actionable with specific steps or a link to an issue tracker.
tests/gui/test_app.py
Outdated
| import sys | ||
|
|
||
| # Skip the test for MacOS due to QWebEngineView ImportError | ||
| # TODO: Need to resolve the ImportError | ||
| if sys.platform == "darwin": | ||
| pytestmark = pytest.mark.skip(reason="ImportError for MacOS") | ||
| pytest.skip(reason="Skipping tests and import on macOS", allow_module_level=True) |
There was a problem hiding this comment.
The conditional block to skip tests on MacOS due to QWebEngineView ImportError is implemented correctly. The TODO comment should be more actionable with specific steps or a link to an issue tracker.
tests/gui/learning/test_dialog.py
Outdated
| import sys | ||
| from qtpy import QtWidgets | ||
|
|
||
| # Skip the test for MacOS due to QWebEngineView ImportError | ||
| # TODO: Need to resolve the ImportError | ||
| if sys.platform == "darwin": | ||
| pytestmark = pytest.mark.skip(reason="ImportError for MacOS") | ||
| pytest.skip(reason="Skipping tests and import on macOS", allow_module_level=True) |
There was a problem hiding this comment.
The conditional block to skip tests on MacOS due to QWebEngineView ImportError is implemented correctly. The TODO comment should be more actionable with specific steps or a link to an issue tracker.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sleap/gui/app.py (5 hunks)
Additional comments: 4
sleap/gui/app.py (4)
56-56: The import statement for
datefrom thedatetimemodule has been added. This is likely used for handling dates within the application, possibly related to the new bulletin feature.71-77: The
BulletinWorkerclass is imported conditionally. If there's anImportError, it falls back to settingonline_bulletintoTrue. This is a good approach to handle optional dependencies, but it's important to ensure that theImportErroris specifically for theBulletinWorkerand not another import issue.Verification successful
The shell script has successfully identified the specific import statement for
BulletinWorkerwithin thesleap/gui/app.pyfile and also in a test filetests/gui/test_app.py. This confirms that theImportErrorhandling in theapp.pyfile is indeed specifically for theBulletinWorkerclass and not due to other import issues.* 183-185: New state variables `announcement last seen date` and `announcement` are added to `self.state`. These are likely used to track the last seen announcement and its content, which is a good practice for the feature being implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that ImportError is specifically for BulletinWorker and not due to other issues. rg --type python "from sleap.gui.dialogs.bulletin import BulletinWorker"Length of output: 105
Script:
#!/bin/bash # Verify that ImportError is specifically for BulletinWorker and not due to other issues. # Correcting the file type flag for Python files. rg --type py "from sleap.gui.dialogs.bulletin import BulletinWorker"Length of output: 221
- 207-213: The logic to check for new announcements and display the bulletin dialog if there is a new announcement is added. This is a key part of the new feature and seems to be correctly placed in the
__init__method to check for announcements when the main window is initialized.
sleap/gui/app.py
Outdated
| def bulletin_dialog(self): | ||
| """Displays bulletin dialog is new announcement is available.""" | ||
| # TODO: Change the URL to the actual SLEAP website before merging to main | ||
| if online_bulletin: | ||
| webbrowser.open('https://sleap.ai/develop/bulletin.html') | ||
| else: | ||
| # Initialize the bulletin popup worker | ||
| popup_worker = BulletinWorker( | ||
| "".join(["# What's New? \n", self.state["announcement"]]), self | ||
| ) | ||
| popup_worker.show_bulletin() | ||
|
|
||
| # Save the bulletin worker so we can close them later | ||
| self._child_windows["bulletin_worker"] = popup_worker # Needed! | ||
|
|
There was a problem hiding this comment.
The bulletin_dialog method is added to display the bulletin dialog if a new announcement is available. It checks the online_bulletin flag to decide whether to open a web browser or to show a bulletin using the BulletinWorker. The TODO comment indicates that the URL needs to be changed before merging, which should be addressed.
- # TODO: Change the URL to the actual SLEAP website before merging to main
+ # Ensure the URL is updated to the actual SLEAP website before merging to mainCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def bulletin_dialog(self): | |
| """Displays bulletin dialog is new announcement is available.""" | |
| # TODO: Change the URL to the actual SLEAP website before merging to main | |
| if online_bulletin: | |
| webbrowser.open('https://sleap.ai/develop/bulletin.html') | |
| else: | |
| # Initialize the bulletin popup worker | |
| popup_worker = BulletinWorker( | |
| "".join(["# What's New? \n", self.state["announcement"]]), self | |
| ) | |
| popup_worker.show_bulletin() | |
| # Save the bulletin worker so we can close them later | |
| self._child_windows["bulletin_worker"] = popup_worker # Needed! | |
| def bulletin_dialog(self): | |
| """Displays bulletin dialog is new announcement is available.""" | |
| # Ensure the URL is updated to the actual SLEAP website before merging to main | |
| if online_bulletin: | |
| webbrowser.open('https://sleap.ai/develop/bulletin.html') | |
| else: | |
| # Initialize the bulletin popup worker | |
| popup_worker = BulletinWorker( | |
| "".join(["# What's New? \n", self.state["announcement"]]), self | |
| ) | |
| popup_worker.show_bulletin() | |
| # Save the bulletin worker so we can close them later | |
| self._child_windows["bulletin_worker"] = popup_worker # Needed! |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/app.py (5 hunks)
- tests/gui/test_app.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- sleap/gui/app.py
- tests/gui/test_app.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/app.py (5 hunks)
- tests/gui/test_app.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- sleap/gui/app.py
- tests/gui/test_app.py
…gelog-announcement-3
*Add helpmenu option
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sleap/gui/app.py (6 hunks)
- sleap/gui/commands.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- sleap/gui/app.py
Additional comments: 2
sleap/gui/commands.py (2)
- 626-629: The method
showBulletinis added to theCommandContextclass to open the latest bulletin. This method is correctly implemented and follows the established pattern for command methods in this file.- 3405-3409: The
ShowBulletinclass is added with ado_actionmethod to execute the bulletin dialog display. The method callscontext.app.bulletin_dialog(), which is presumably the method that handles the dialog creation and display. This is consistent with the design of other commands in the file.
Description
Creating a new file
bulletin.pyand classBulletinDialogto display the announcement. An instance of this BulletinDialog class is created while initializing theMainWindowif there is an unseen announcement.This will create a bulletin pop-up that will display the latest information that is found in the
bulletin.mdfile.Types of changes
Does this address any currently open issues?
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
New Features
Tests
Chores