Improve theming #1151
Improve theming #1151
Conversation
use plain Qt widgets instead
* elide sender name and add tooltip with full name * fix breaking lines with non-breaking space at the end of sender name -- move this space to text body
seems like `append` works just fine
just use the common one
* load only one common stylesheet file * load stylesheet file once * take all hardcoded styles from .css file the one left to integrate is chat channel CSS, but it's a jinja template too, so it will be more difficult/impossible
📝 WalkthroughWalkthroughCentralized theme API and stylesheet usage; many UI .ui enums modernized; extensive theme-driven color migrations; added chat search/highlighting and sender elision; news WPAPI refactor to JsonApiBase; removed legacy CSS/templates and per-widget stylesheet loaders. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Channel as "ChannelWidget"
participant Highlighter as "SearchHighlighter"
participant Theme as "ThemeSet"
participant Doc as "QTextDocument"
User->>Channel: Press Ctrl+F
Channel->>Channel: _toggle_search() (show search_frame, focus edit)
User->>Channel: Enter term, press Enter
Channel->>Highlighter: update(term)
Highlighter->>Theme: find_stylesheet_attribute("SearchHighlighter::custom","highlight-color")
Theme-->>Highlighter: color value
Highlighter->>Doc: apply QTextCharFormat with color to matches
Highlighter-->>Channel: return matches/count
Channel->>Channel: _navigate_to_search_result(index) (move cursor)
Channel-->>User: Highlighted result shown, label updated (current/total)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/news/wpapi.py (1)
22-39: Normalize WPAPI fields to avoid AttributeError/TypeError in downstream code.
excerpt.strip()is called at line 88 of_newswidget.py, andauthor[0]['name']is accessed at line 82 ofnewsitem.py. If the WordPress API omits these fields, both will crash. Add safe defaults at ingest:Suggested fix
posts = [ { - 'title': post.get('title', {}).get('rendered'), - 'body': post.get('content', {}).get('rendered'), - 'date': post.get('date'), - 'excerpt': post.get('excerpt', {}).get('rendered'), - 'author': post.get('_embedded', {}).get('author'), - 'link': post.get('link'), - 'external_link': post.get('newshub_externalLinkUrl'), + 'title': post.get('title', {}).get('rendered') or "", + 'body': post.get('content', {}).get('rendered') or "", + 'date': post.get('date') or "", + 'excerpt': post.get('excerpt', {}).get('rendered') or "", + 'author': post.get('_embedded', {}).get('author') or [{"name": "Unknown"}], + 'link': post.get('link') or "", + 'external_link': post.get('newshub_externalLinkUrl') or "", 'img_url': ( post.get('_embedded', {}) .get('wp:featuredmedia', [{}])[0] - .get('source_url', "") + .get('source_url', "") or "" ), } for post in news ]src/replays/replayitem.py (1)
265-287: Reset state/live_delay whenendTimebecomes available.If a replay transitions from live to finished,
statecan remainPLAYINGandlive_delaycan stayTrue, which then misroutes UI logic (details button + double-click behavior).🛠️ Proposed fix
if replay["endTime"] is None: + self.live_delay = False seconds = time.time() - startDt.timestamp() if seconds > 86400: # more than 24 hours self.duration = "" self.state = ReplayItemState.MISSING elif seconds > 7200: # more than 2 hours self.duration = time.strftime('%H:%M:%S', time.gmtime(seconds)) self.state = ReplayItemState.UNKNOWN elif seconds < 300: # less than 5 minutes self.duration = time.strftime('%H:%M:%S', time.gmtime(seconds)) self.state = ReplayItemState.DELAY self.live_delay = True else: self.duration = time.strftime('%H:%M:%S', time.gmtime(seconds)) self.state = ReplayItemState.PLAYING else: endDt = datetime.strptime(replay["endTime"], '%Y-%m-%dT%H:%M:%SZ') # local time endDt = endDt.replace(tzinfo=timezone.utc).astimezone(tz=None) self.duration = time.strftime( '%H:%M:%S', time.gmtime((endDt - startDt).total_seconds()), ) + self.state = ReplayItemState.FINISHED + self.live_delay = False
🤖 Fix all issues with AI agents
In `@res/client/client.css`:
- Around line 996-999: The Qt stylesheet uses the sub-control form ::hover which
doesn't match pseudo-states; change all occurrences of ::hover to the
single-colon :hover for the QPushButton selectors (QPushButton#searchButton,
`#advSearchButton`, `#fafDbButton`, `#spookyDbButton`) so the hover rules apply; also
update the other instances flagged (lines around the second block at 1011-1014)
to use :hover consistently.
In `@res/themes/Dark` Blue/client/client.css:
- Around line 991-1000: The QToolButton::checked CSS rule contains a duplicate
border declaration: remove the first "border: none" from the
QToolButton::checked selector so only "border: 2px solid `#374151`" remains,
leaving the rest of the properties (color, padding, background-color,
border-radius) unchanged.
In `@res/themes/unthemed/client/client_light.css`:
- Around line 152-155: The QSS rule for QTableView has an invalid terminator —
change the trailing comma after the outline property to a semicolon so the
declaration reads outline: none; inside the QTableView selector (locate the
QTableView block in client_light.css and update the outline property
terminator).
In `@res/themes/unthemed/client/client.css`:
- Around line 159-162: The QSS rule for the QTableView selector uses an invalid
comma after the property; in the QTableView block (selector "QTableView")
replace the trailing comma in the property declaration "outline: none," with a
semicolon so it becomes "outline: none;"; ensure the property line ends with a
semicolon to conform to QSS syntax.
In `@src/chat/channel_widget.py`:
- Around line 300-303: The search term is used directly to build a
QRegularExpression which can fail or behave unexpectedly for metacharacters;
update the code that sets the expression (the call to QRegularExpression(...)
used by self.highlighter.set_expression) to escape the user input first using
QRegularExpression.escape(self.search_term) before constructing the
QRegularExpression, then call self.highlighter.rehighlight() as before so the
highlighter always receives a literal-safe pattern.
In `@src/client/playercolors.py`:
- Line 27: The call in PlayerColors.__init__ that assigns self.colors =
self._theme.find_stylesheet_style_as_dict("PlayerColors::custom") can raise if
the "PlayerColors::custom" section is missing; update the code in the
PlayerColors class to catch that case by checking for existence (or catching the
specific exception) from find_stylesheet_style_as_dict, and provide a clear
fallback (e.g., a default color dict) or raise a descriptive error that mentions
the missing "PlayerColors::custom" section and how to fix it; reference the
find_stylesheet_style_as_dict call and the self.colors assignment so the change
either validates presence before using it or wraps the call and substitutes a
default map with a logged/raised message.
In `@src/config/version.py`:
- Line 102: The stderr write in version.py currently uses
sys.stderr.write(f"Error grabbing git version: {e}") without a trailing newline;
update the sys.stderr.write call (the one that logs the exception when grabbing
git version) to append os.linesep (or '\n')—matching the style used later on
line 134—so the error message ends with a newline and subsequent shell output
appears on the next line.
In `@src/games/gamepanelwidget.py`:
- Around line 260-262: The f-string for fmod_text is treating
"self.fmod_color_accent" as literal text instead of the variable; update the
construction of fmod_text in the branch that checks new.featured_mod != "faf" so
it interpolates the attribute (self.fmod_color_accent) into the string, e.g.
reference the variable inside the f-string used before calling
self.ui.featuredModLabel.setText(fmod_text) so the actual color value is
inserted rather than the literal name.
In `@src/games/hostgamewidget.py`:
- Around line 194-195: The name filter currently bypasses the mod-type filter
because textChanged is wired to filter_mods_by_name alone; change to a single
unified filter routine (e.g., apply_mod_filters) and update both the name filter
connection (self.ui.modNameFilter.textChanged.connect) and the mod-type toggle
handlers to call apply_mod_filters instead of
filter_mods_by_name/filter_mods_by_type; implement apply_mod_filters to evaluate
both predicates (name contains text AND matches active mod-type toggles) for
each item so typing never overrides the type filter.
In `@src/games/mapgenoptions.py`:
- Around line 94-99: In populate(), the enumerate(index) does not match the
combobox position when items already exist so setItemData(index, ...) applies to
wrong entries; fix by either clearing the combobox at the start of populate()
(call self.ui_elem.clear()) before the loop, or by using the combo's actual
index after adding each item (e.g., call self.ui_elem.addItem(opt) then compute
newly_added_index = self.ui_elem.count() - 1 and call
self.ui_elem.setItemData(newly_added_index, opt, Qt.ItemDataRole.ToolTipRole));
update the populate() method accordingly, referencing populate,
self.ui_elem.addItem and self.ui_elem.setItemData with
Qt.ItemDataRole.ToolTipRole.
In `@src/news/_newswidget.py`:
- Around line 141-149: In open_news_in_browser, guard against None as well as
empty strings for newsPost["external_link"] and newsPost["link"] by treating
them as falsy; pick external_link = current.newsPost.get("external_link") or
current.newsPost.get("link") and if that value is None or an empty string return
early, then construct a QUrl(external_link) and only call
QDesktopServices.openUrl if qurl.isValid() is True; reference current =
self.newsList.currentItem(), current.newsPost["external_link"],
current.newsPost["link"], QUrl and QDesktopServices.openUrl when making the
change.
- Around line 63-69: Clear the image label when starting a new download and on
download failure to avoid showing stale pixmaps: in itemChanged (the method that
begins a new image download for a newly selected news item) call
self.imageLabel.clear() before initiating the download, and in
item_image_downloaded add a branch where if download_failed is True you call
self.imageLabel.clear() (and then proceed to self.show_newspage()). This ensures
old images are removed both when a new item is selected and when the download
fails.
In `@src/playercard/plot.py`:
- Line 25: The constructor annotation for LineSeries in the __init__ signature
uses the class before it's defined; fix by using a forward reference: change the
type annotation to a string literal 'LineSeries' in def __init__(self,
plotwidget: pg.PlotWidget, series: 'LineSeries') -> None or alternatively enable
postponed evaluation for all annotations by adding from __future__ import
annotations at the top of the file so the existing signature can remain
unchanged; update the annotation in the __init__ method of the class containing
that constructor accordingly.
In `@src/replays/_replayswidget.py`:
- Around line 399-408: The title_color_str is incorrectly fetched using the
"time-color" key (copy/paste error) so LocalReplayTreeItem is ignoring a
dedicated title color; change the second util.THEME.find_stylesheet_attribute
call (that assigns title_color_str) to request the "title-color" attribute, and
keep a fallback to time_color_str if "title-color" is not found so the
setForeground calls use the correct color values.
In `@src/replays/replayitem.py`:
- Around line 218-227: The _colors method assumes keys "status_color_delay" and
"status_color_missing" exist and will raise KeyError for custom/incomplete
themes; update _colors (which calls replay_item_colors()) to lookup those keys
with a safe fallback (e.g., use colors.get("status_color_delay",
colors.get("status_color", "")) and colors.get("status_color_missing",
colors.get("status_color", ""))) when setting colors["status_color"] for
ReplayItemState.DELAY and ReplayItemState.MISSING, so rendering remains
resilient if theme keys are absent.
In `@src/replays/replayToolbox.py`:
- Around line 302-304: The call to widgetHandler.apiConnector.get_parsed(...)
only provides the success callback (widgetHandler.process_replays_data) and thus
swallows API errors; update the call to pass the standard search-path error
handler as the third argument to get_parsed so failures are propagated and the
UI can clear the “searching” state and show errors (use the same error handler
used by your normal search flow, e.g. widgetHandler.process_search_error or the
equivalent error handler method on widgetHandler).
- Around line 155-163: THEME.find_stylesheet_style(...) can return None which
will crash when passed to QWidget.setStyleSheet; update the assignments for
scroll_sheet and button_sheet (the calls to THEME.find_stylesheet_style in
ReplayToolBox) to guard against None by falling back to an empty string (e.g.,
use the returned value or "" ) before calling children[-1].setStyleSheet and
children[-2].setStyleSheet so setStyleSheet always receives a str.
In `@src/util/theme.py`:
- Around line 469-473: The method find_stylesheet_style currently ignores the
fallback parameter; update its logic so it returns the first non-None value
among self._theme.find_stylesheet_style(section),
self._default_theme.find_stylesheet_style(section), and the provided fallback
string (fallback) — i.e., call self._theme.find_stylesheet_style(section), if
None call self._default_theme.find_stylesheet_style(section), and if that is
also None return fallback; ensure the function signature and return type remain
unchanged and reference the existing symbols find_stylesheet_style, self._theme,
self._default_theme, and fallback.
- Around line 475-479: find_stylesheet_style_as_dict calls find_stylesheet_style
which may return None, causing .splitlines() to raise AttributeError; change the
function to treat a None result as an empty string (or return an empty dict
early) before calling splitlines(), then ensure the comprehension handles empty
sheetlines safely (e.g., skip empty lines or guard the line.split(":") to avoid
ValueError). Update find_stylesheet_style_as_dict to use the result of
find_stylesheet_style(section) or "" and return an empty dict when there are no
valid "name:value" lines.
- Around line 163-182: Add unit tests for the stylesheet parsing edge cases
around find_stylesheet_attribute and find_stylesheet_style: cover when
self.stylesheet is None (assert returns None), when the requested section is
missing (returns None), when the CSS is malformed (missing braces or semicolons
— assert safe None or no crash), and attributes whose values include special
characters (e.g., parentheses, commas, colons, unicode, and additional
whitespace/newlines) to ensure the regex still extracts the correct group;
create focused test cases that construct Theme instances with those stylesheet
strings and assert the expected return values from find_stylesheet_attribute and
find_stylesheet_style.
🧹 Nitpick comments (19)
src/config/version.py (1)
59-71: Consider using a context manager for file reading.The
try/finallyblock correctly ensures the file is closed, but awithstatement would be more idiomatic and concise.♻️ Suggested refactor
def read_version_file(dir): try: - f = open(version_filename(dir)) - - try: - version = f.readlines()[0] - return version.strip() - - finally: - f.close() - + with open(version_filename(dir)) as f: + version = f.readlines()[0] + return version.strip() except OSError: return Nonesrc/coop/_coopwidget.py (1)
56-59: Consider providing a fallback color.If the stylesheet doesn't define
CoopWidgetTreeRootItem::customwith acolorattribute,find_stylesheet_attributereturns an empty string. This would result incolor=''in the HTML, which may render with an unpredictable default color.Since this previously used hardcoded white, consider adding a fallback:
Proposed fix
self._coop_type_item_color = util.THEME.find_stylesheet_attribute( "CoopWidgetTreeRootItem::custom", "color", + "white", )src/news/_newswidget.py (1)
63-63: Mark the unused slot argument as intentional.
image_nameisn’t referenced; renaming to_image_namedocuments intent and avoids unused‑arg noise.♻️ Suggested tweak
-def item_image_downloaded(self, image_name: str, result: tuple[str, bool]) -> None: +def item_image_downloaded(self, _image_name: str, result: tuple[str, bool]) -> None:src/news/newsitem.py (1)
89-92: Consider returningNotImplementedfor incompatible types.Returning
Falsewhenotheris not aNewsItemmay silently produce incorrect sorting behavior. The Python data model convention is to returnNotImplementedso Python can try the reflected operation on the other operand.♻️ Proposed fix
def __lt__(self, other: object) -> bool: if not isinstance(other, NewsItem): - return False + return NotImplemented return self.newsPost['date'].__lt__(other.newsPost['date'])src/replays/replaydetails/rangeslider.py (2)
151-156: Consider providing a fallback color.If the stylesheet attribute is not found,
find_stylesheet_attributereturns an empty string, andQColor("")creates an invalid color which may cause rendering issues. Additionally, this lookup happens on everypaintEvent, which could have minor performance implications.Consider caching the color or providing a fallback:
♻️ Suggested approach
+ color_str = THEME.find_stylesheet_attribute( + "QSlider::groove:horizontal", + "background", + fallback="#3a3a3a", # sensible default + ) - color_str = THEME.find_stylesheet_attribute( - "QSlider::groove:horizontal", - "background", - )Alternatively, cache the colors in
__init__or connect toTHEME.stylesheets_reloadedto update them only when the theme changes.
204-209: Same consideration for handle color fallback.Similar to the groove color, this lookup returns an empty string if not found. A fallback color would ensure consistent rendering across all themes.
src/games/automatchframe.py (2)
213-220: Consider providing a fallback color for robustness.If the theme stylesheet doesn't define
searching-tab-color,find_stylesheet_attributereturns an empty string, andQColor("")creates an invalid color that may cause the tab text to not render correctly.Suggested fix with fallback
- color_str = util.THEME.find_stylesheet_attribute( - "AutomatchTabs::custom", - "searching-tab-color", - ) + color_str = util.THEME.find_stylesheet_attribute( + "AutomatchTabs::custom", + "searching-tab-color", + fallback="orange", + )
229-234: Consider providing a fallback color for the default tab color.Same concern as above - if
default-tab-coloris not defined in the stylesheet, the tab text color may not render correctly.Suggested fix with fallback
- color_str = util.THEME.find_stylesheet_attribute( - "AutomatchTabs::custom", - "default-tab-color", - ) + color_str = util.THEME.find_stylesheet_attribute( + "AutomatchTabs::custom", + "default-tab-color", + fallback="silver", + )src/games/_gameswidget.py (1)
454-458: Consider providing a fallback color for consistency with other usages.For robustness, provide a fallback value in case the stylesheet attribute is not defined, similar to the suggestion for
automatchframe.py.Suggested fix
- tab_color_str = util.THEME.find_stylesheet_attribute( - "AutomatchTabs::custom", - "default-tab-color", - ) + tab_color_str = util.THEME.find_stylesheet_attribute( + "AutomatchTabs::custom", + "default-tab-color", + fallback="silver", + )src/notifications/__init__.py (1)
57-64: Consider providing fallback colors for notification styling.If the stylesheet doesn't define these attributes, empty strings will be used in HTML
colorattributes (e.g.,color=""), which may result in unpredictable text rendering.Suggested fix with fallbacks
self.secondary_color = util.THEME.find_stylesheet_attribute( "Notifications::custom", "secondary-color", + fallback="silver", ) self.mods_color = util.THEME.find_stylesheet_attribute( "Notifications::custom", "mods-color", + fallback="grey", )src/vaults/reviewwidget.py (1)
174-179: Consider providing a fallback color for star fill.If
star-fill-coloris not defined in the stylesheet,QColor("")creates an invalid color, which may cause the stars to not render correctly.Suggested fix with fallback
STAR_FILL_COLOR = QColor( THEME.find_stylesheet_attribute( "ReviewWidget::custom", "star-fill-color", + fallback="#4CAF50", ), )src/replays/replaydetails/tabs/events_tab.py (1)
106-117: Consider providing fallback colors for event item rendering.If these stylesheet attributes are not defined,
QColor("")creates invalid colors, which may cause the event items to render incorrectly.Suggested fix with fallbacks
TEXT_RECT_COLOR = QColor( util.THEME.find_stylesheet_attribute( "EventItemDelegate::custom", "text-rect-color", + fallback="#202025", ), ) TEAM_RECT_COLOR = QColor( util.THEME.find_stylesheet_attribute( "EventItemDelegate::custom", "team-rect-color", + fallback="#202025", ), )src/playercard/plot.py (2)
12-23: Consider providing fallback colors for crosshair styling.Without fallbacks, if the stylesheet attributes are not defined,
pg.mkPenandpg.TextItemmay receive empty strings, potentially causing rendering issues.Suggested fix with fallbacks
COLOR = THEME.find_stylesheet_attribute( "RatingPlotCrosshairs::custom", "line-color", + fallback="green", ) TEXT_COLOR = THEME.find_stylesheet_attribute( "RatingPlotCrosshairs::custom", "text-color", + fallback="black", ) BACKGROUND_COLOR = THEME.find_stylesheet_attribute( "RatingPlotCrosshairs::custom", "background-color", + fallback="yellow", )
180-195: Consider providing fallback colors for plot widget styling.Same concern as above - provide fallbacks for
background-colorandcolorattributes to ensure the plot renders correctly even if the stylesheet doesn't define these values.Suggested fix with fallbacks
background = THEME.find_stylesheet_attribute( "RatingPlotWidget::custom", "background-color", + fallback="#1e1e1e", ) self.widget.setBackground(background) self.widget.setAxisItems({"bottom": pg.DateAxisItem()}) self.series = LineSeries() self.crosshairs = Crosshairs(self.widget, self.series) self.hide_scene_actions() self.hide_irrelevant_plot_actions() self.add_custom_menu_actions() pen_color = THEME.find_stylesheet_attribute( "RatingPlotWidget::custom", "color", + fallback="orange", )src/replays/_replayswidget.py (1)
1070-1070: PreferReplayItemStatechecks over parsing HTML status.String matching couples logic to presentation and is fragile if status text changes. Use the enum-driven state instead.
🛠️ Proposed refactor
-from src.replays.replayitem import ReplayItem -from src.replays.replayitem import ReplayItemDelegate +from src.replays.replayitem import ReplayItem +from src.replays.replayitem import ReplayItemDelegate +from src.replays.replayitem import ReplayItemState @@ - game_finished = "playing" not in item.status + is_playing = item.state in { + ReplayItemState.DELAY, + ReplayItemState.PLAYING, + ReplayItemState.UNKNOWN, + } + game_finished = not is_playing @@ - if "playing" in item.status: # live game will not be in vault + if item.state in { + ReplayItemState.DELAY, + ReplayItemState.PLAYING, + ReplayItemState.UNKNOWN, + }: # live game will not be in vaultAlso applies to: 1097-1097
res/themes/Dark Blue/client/client.css (2)
614-623: Minor: Shorthand property override.The
backgroundshorthand on line 617 overrides thebackground-coloron line 616. If both values are identical, simplify to justbackground:QScrollArea#replayScoreScrollArea { - background-color: `#1A1B2C`; background: `#1A1B2C`; color: `#1A1B2C`;
636-717: Remove commented-out scrollbar code.This large block of commented-out CSS adds noise without providing value. If these are alternative styling options being preserved for reference, consider moving them to a separate file or documenting the rationale. Otherwise, remove them to improve maintainability.
src/chat/channel_view.py (2)
272-279: Lazily initializeQFontMetricsafter the GUI is ready.
CreatingQFontMetricsat import time risks constructing GUI resources before aQGuiApplicationexists and won’t reflect font/theme changes. Prefer instance-level initialization.♻️ Suggested adjustment
-class ChatLineFormatter: - _font_metrics = QFontMetrics(QFont()) +class ChatLineFormatter: def __init__(self, theme: ThemeSet, player_colors: PlayerColors) -> None: self._set_theme(theme) self._player_colors = player_colors self._last_timestamp = None + self._font_metrics = QFontMetrics(QFont())
335-366: Elide using raw sender text to avoid entity-length skew.
elidedTextmeasures characters; if you pass an HTML‑escaped sender, names containing&,<, etc. can be truncated too early. Compute elision on the raw sender and escape afterward.♻️ Suggested adjustment
- sender_name = self._sender_name(data) - elided_sender = self._font_metrics.elidedText(sender_name, Qt.TextElideMode.ElideRight, 99) + sender_raw = self._sender_name(data) + elided_raw = self._font_metrics.elidedText(sender_raw, Qt.TextElideMode.ElideRight, 99) + sender_name = html.escape(sender_raw) + elided_sender = html.escape(elided_raw) @@ def _sender_name(self, data: ChatLineMetadata) -> str: if data.line.sender is None: return "" sender = ChatterFormat.name(data.line.sender, data.meta.player.clan()) - return html.escape(sender) + return senderAlso applies to: 380-385
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/util/theme.py`:
- Around line 60-68: The stylesheet() cached_property currently checks self.name
is None to detect "unthemed" but the canonical marker is themedir per the
docstring; change the conditional in stylesheet() to use self.themedir (e.g., if
self.themedir and QtWidgets.QApplication.style().name() == "windowsvista") so
the light stylesheet is selected when the instance is marked unthemed (including
cases where name is empty); keep the rest of the logic and continue to call
readstylesheet("client/client_light.css") vs readstylesheet("client/client.css")
as before.
- Around line 406-408: The readstylesheet method currently calls
_theme_callchain without the required themed argument; update readstylesheet to
accept a themed: bool parameter (e.g., def readstylesheet(self, filename: str,
themed: bool = True)) and forward that flag to _theme_callchain by calling
self._theme_callchain("readstylesheet", filename, themed) so the signature and
callsite match the expected parameters.
♻️ Duplicate comments (3)
src/util/theme.py (3)
163-182: Add edge‑case tests for regex‑based stylesheet lookups.These regex helpers are brittle without coverage for malformed CSS, missing sections, and special characters.
469-473: Fallback parameter is still ignored.If both theme and default theme lack the section,
fallbackshould be returned.
475-480: Guard againstNone/ invalid lines before split.
find_stylesheet_style()can returnNone; calling.splitlines()will raise without a guard.
🧹 Nitpick comments (3)
src/util/theme.py (3)
132-139: Return type should allowNonefrom_noneIfNoFile.The decorator can return
Nonewhen the file is missing; the annotation should reflect that.♻️ Suggested tweak
- def readstylesheet(self, filename: str) -> str: + def readstylesheet(self, filename: str) -> str | None:
288-301: _do_setTheme return type should bebool.The method returns
theme_changed()and callers treat it as a boolean; update the annotation to match.♻️ Suggested tweak
- def _do_setTheme(self, new_theme: Theme) -> None: + def _do_setTheme(self, new_theme: Theme) -> bool:
429-434: Avoid recomputing cached stylesheets during reload.
hasattrtriggers the cached_property; evict from__dict__to avoid unnecessary IO.♻️ Suggested tweak
- if hasattr(self._theme, "stylesheet"): - del self._theme.stylesheet - if hasattr(self._default_theme, "stylesheet"): - del self._default_theme.stylesheet + self._theme.__dict__.pop("stylesheet", None) + self._default_theme.__dict__.pop("stylesheet", None)
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
res/client/client.css (1)
754-758: Fix QScrollBar arrow sub-control selectors to use double colons.Qt CSS requires
::for sub-controls, not:. UpdateQScrollBar:up-arrow,QScrollBar:down-arrow,QScrollBar:right-arrow, andQScrollBar:left-arrowto use double colons (QScrollBar::up-arrow, etc.).✅ Proposed fix
-QScrollBar:down-arrow { +QScrollBar::down-arrow { background-image: url('%THEMEPATH%/client/scrollDown.png'); background-position: center center; background-repeat:no-repeat; } -QScrollBar:up-arrow { +QScrollBar::up-arrow { background-image: url('%THEMEPATH%/client/scrollUp.png'); background-position: center center; background-repeat:no-repeat; } -QScrollBar:right-arrow { +QScrollBar::right-arrow { background-image: url('%THEMEPATH%/client/scrollRight.png'); background-position: center center; background-repeat:no-repeat; } -QScrollBar:left-arrow { +QScrollBar::left-arrow { background-image: url('%THEMEPATH%/client/scrollLeft.png'); background-position: center center; background-repeat:no-repeat; }
🤖 Fix all issues with AI agents
In `@res/themes/Dark` Blue/client/client.css:
- Around line 556-565: Fix the QSS pseudo-state syntax for the
QListWidget#teamList rules: replace occurrences of the incorrect
"::item::selected" and "::item::selected:!active" with the proper
"::item:selected" and "::item:selected:!active" respectively so the selectors
target item pseudo-states correctly for the QListWidget#teamList styles.
- Around line 954-1013: Replace the incorrect double-colon pseudo-state
selectors with single-colon pseudo-states: change
QToolButton#rankedPlay::disabled to QToolButton#rankedPlay:disabled,
QToolButton::pressed to QToolButton:pressed, QToolButton::checked to
QToolButton:checked, and QToolButton#gamePanelButton::checked to
QToolButton#gamePanelButton:checked so the Qt stylesheet uses proper
pseudo-state syntax (double-colons remain only for subcontrols like
::menu/::indicator).
- Around line 104-108: The QSS selectors use the invalid double-colon
pseudo-state `::selected` (e.g., in the selector "QTabWidget >
QTabBar::tab::selected") — replace `::selected` with the correct single-colon
pseudo-state `:selected` in all occurrences (including the selectors around
lines referencing QTabWidget > QTabBar::tab::selected and similar entries at the
other noted spots) so the selected-tab styling matches Qt's QSS syntax; update
each selector like "QTabBar::tab::selected" to "QTabBar::tab:selected"
accordingly.
In `@res/themes/unthemed/client/client_light.css`:
- Around line 330-371: The QSS uses double-colon for pseudo-states which won't
match; update the selectors so pseudo-states use a single colon. Specifically,
change QToolButton#rankedPlay::disabled to QToolButton#rankedPlay:disabled,
QToolButton::pressed to QToolButton:pressed, QToolButton::checked to
QToolButton:checked, and QToolButton#gamePanelButton::checked to
QToolButton#gamePanelButton:checked (leave any true sub-control selectors with
:: intact).
In `@res/themes/unthemed/client/client.css`:
- Around line 356-397: Replace double-colon pseudo-state selectors with
single-colon for QToolButton rules so QSS applies correctly: change
QToolButton#rankedPlay::disabled to QToolButton#rankedPlay:disabled,
QToolButton::pressed to QToolButton:pressed, QToolButton::checked to
QToolButton:checked, and QToolButton#gamePanelButton::checked to
QToolButton#gamePanelButton:checked (leave sub-control selectors unchanged if
any).
♻️ Duplicate comments (1)
res/client/client.css (1)
978-1033: Fix remaining::hoverselectors (Qt uses:hover).These hover rules won’t match with
::hover. Please switch to:hoverfor all selectors in these groups.🛠️ Proposed fix
-QPushButton#refreshButton:hover, `#nextButton`::hover, `#previousButton`::hover, `#goToPageButton`::hover, -#firstButton:hover, `#lastButton`::hover, `#resetButton`::hover, `#searchPlayerButton`::hover, -#UIButton:hover, `#uploadButton`::hover, `#kickButton`::hover, `#leaveButton`::hover +QPushButton#refreshButton:hover, `#nextButton`:hover, `#previousButton`:hover, `#goToPageButton`:hover, +#firstButton:hover, `#lastButton`:hover, `#resetButton`:hover, `#searchPlayerButton`:hover, +#UIButton:hover, `#uploadButton`:hover, `#kickButton`:hover, `#leaveButton`:hover { background-color: `#808080`; } -QPushButton#searchButton:hover, `#advSearchButton`::hover, `#fafDbButton`::hover, `#spookyDbButton`::hover +QPushButton#searchButton:hover, `#advSearchButton`:hover, `#fafDbButton`:hover, `#spookyDbButton`:hover { background-color: `#808080`; } -QPushButton#RefreshResetButton:hover, `#advResetButton`::hover +QPushButton#RefreshResetButton:hover, `#advResetButton`:hover { background-color: `#808080`; } -QPushButton#saveMapGenSettingsButton:hover, `#resetMapGenSettingsButton`::hover,`#generateMapButton`::hover, +QPushButton#saveMapGenSettingsButton:hover, `#resetMapGenSettingsButton`:hover,`#generateMapButton`:hover, `#hostGameButton`:hover, `#saveHostGameSettingsButton`:hover {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@res/themes/Dark` Blue/client/client.css:
- Around line 110-128: Replace invalid 'none' color values in the QSS selectors
for QTabWidget#matchmakerQueues: change background-color: none to
background-color: transparent (or remove it) and remove or replace color: none
with a valid color (or inherit/initial) in the QTabWidget#matchmakerQueues >
QTabBar::tab rule; update the rules for QTabWidget#matchmakerQueues::pane and
QTabWidget#matchmakerQueues > QTabBar::tab to use transparent or valid color
values so the stylesheet is accepted by Qt.
♻️ Duplicate comments (4)
src/games/hostgamewidget.py (1)
192-195: Name filter still overrides type filter in some states.
Same issue as previously noted; consider a unified filter so text changes never bypass the active type toggle.Also applies to: 352-368
res/themes/unthemed/client/client.css (1)
361-402: Use single-colon pseudo-states for QToolButton states (duplicate).The
::disabled/::pressed/::checkedselectors won’t match in QSS; these should be:disabled/:pressed/:checked.✏️ Proposed fix
-QToolButton#rankedPlay::disabled +QToolButton#rankedPlay:disabled { border-radius: 5px; border: 1px solid; } -QToolButton::pressed +QToolButton:pressed { border: none; padding:5px; border-radius : 5px; } -QToolButton::checked +QToolButton:checked { border: 2px solid; padding:5px; border-radius : 5px; } -QToolButton#gamePanelButton::checked +QToolButton#gamePanelButton:checked { border: none; padding: 2px; }res/themes/unthemed/client/client_light.css (1)
335-373: Use single-colon pseudo-states for QToolButton states (duplicate).These QSS selectors won’t match with double-colon pseudo-states.
✏️ Proposed fix
-QToolButton#rankedPlay::disabled +QToolButton#rankedPlay:disabled { border-radius: 5px; border: 1px solid; } -QToolButton::pressed +QToolButton:pressed { border: none; padding:5px; border-radius : 5px; } -QToolButton::checked +QToolButton:checked { border: 2px solid; padding:5px; border-radius : 5px; } -QToolButton#gamePanelButton::checked +QToolButton#gamePanelButton:checked { border: none; padding: 2px; }res/client/client.css (1)
980-982: Use:hover(single colon) for Qt pseudo-states.
::hoveris treated as a sub-control, so these hover rules won’t match.🛠️ Suggested fix
-QPushButton#refreshButton:hover, `#nextButton`::hover, `#previousButton`::hover, `#goToPageButton`::hover, -#firstButton:hover, `#lastButton`::hover, `#resetButton`::hover, `#searchPlayerButton`::hover, -#UIButton:hover, `#uploadButton`::hover, `#kickButton`::hover, `#leaveButton`::hover +QPushButton#refreshButton:hover, `#nextButton`:hover, `#previousButton`:hover, `#goToPageButton`:hover, +#firstButton:hover, `#lastButton`:hover, `#resetButton`:hover, `#searchPlayerButton`:hover, +#UIButton:hover, `#uploadButton`:hover, `#kickButton`:hover, `#leaveButton`:hover -QPushButton#searchButton:hover, `#advSearchButton`::hover, `#fafDbButton`::hover, `#spookyDbButton`::hover +QPushButton#searchButton:hover, `#advSearchButton`:hover, `#fafDbButton`:hover, `#spookyDbButton`:hover -QPushButton#RefreshResetButton:hover, `#advResetButton`::hover +QPushButton#RefreshResetButton:hover, `#advResetButton`:hover -QPushButton#saveMapGenSettingsButton:hover, `#resetMapGenSettingsButton`::hover,`#generateMapButton`::hover, +QPushButton#saveMapGenSettingsButton:hover, `#resetMapGenSettingsButton`:hover, `#generateMapButton`:hover,Also applies to: 997-1000, 1012-1014, 1033-1035
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@res/themes/Dark` Blue/client/client.css:
- Around line 1603-1622: In PlayerColors::custom update the invalid CSS color
names by replacing "medium orchid" with the correct single-word CSS color
"mediumorchid" for the friend_mod and self_mod properties so the Qt stylesheet
parser recognizes them; locate the PlayerColors::custom block and change the
values for friend_mod and self_mod accordingly.
♻️ Duplicate comments (1)
src/util/theme.py (1)
57-66: Unthemed detection condition may never match.The
stylesheetproperty checksself.name is Noneto detect unthemed mode (line 60), but the unthemed Theme is created withTheme(None, '')at line 201—wherenameis an empty string'', notNone. This means the light stylesheet path will never be taken for the unthemed case.Per the docstring convention,
_themediris the canonical "no theme" marker. Consider usingself._themedir is Noneinstead.🐛 Proposed fix
`@cached_property` def stylesheet(self) -> str | None: if ( - self.name is None # unthemed + self._themedir is None # unthemed and QtWidgets.QApplication.style().name() == "windowsvista" ): return self.readstylesheet("client/client_light.css") else: return self.readstylesheet("client/client.css")
🧹 Nitpick comments (5)
res/themes/Dark Blue/client/client.css (2)
614-623: Shorthandbackgroundoverrides the precedingbackground-color.Line 617 uses the shorthand
backgroundproperty right afterbackground-coloron line 616, which completely overrides it. This is redundant—keep only one.♻️ Proposed cleanup
QScrollArea#replayScoreScrollArea { - background-color: `#1A1B2C`; background: `#1A1B2C`; color: `#1A1B2C`; border-width: 0px; border-bottom-left-radius: 0px; border-bottom-right-radius: 0px; padding: 0px; }
637-717: Consider removing the large block of commented-out code.~80 lines of commented-out scrollbar styling exist here. If this code is no longer needed, removing it would improve readability. If it's kept for reference, consider moving it to documentation or a separate file.
src/util/theme.py (1)
422-427: Consider checking__dict__directly to avoid unnecessary computation.Using
hasattr()on acached_propertymay trigger computation of the property value. A more efficient approach is to check if the cached value exists in the instance's__dict__:♻️ Proposed refactor
def reloadStyleSheets(self) -> None: - if hasattr(self._theme, "stylesheet"): - del self._theme.stylesheet - if hasattr(self._default_theme, "stylesheet"): - del self._default_theme.stylesheet + if "stylesheet" in self._theme.__dict__: + del self._theme.__dict__["stylesheet"] + if "stylesheet" in self._default_theme.__dict__: + del self._default_theme.__dict__["stylesheet"] self.stylesheets_reloaded.emit()src/replays/_replayswidget.py (2)
1070-1070: Avoid string-matching UI status text for replay state logic.
item.statusis UI HTML/text and can change; usingitem.state/ReplayItemState(or a helper likeitem.is_playing) is more robust. This affects Line 1070 and Line 1097.Also applies to: 1097-1097
1250-1264: Prefer setForeground over HTML tags for bucket header colors (or verify rich-text rendering).
If the delegate doesn’t render rich text for bucket headers, the<font>tags will show literally. A safer path is to set the foreground color directly.♻️ Suggested change
- bucket_item.setText( - 0, f"<font color='{self.bucket_item_date_color}'>{bucket}</font>", - ) + bucket_item.setText(0, bucket) + bucket_item.setForeground(0, QtGui.QColor(self.bucket_item_date_color)) bucket_len = len(buckets[bucket]) - bucket_item.setText( - 1, f"<font color='{self.bucket_item_count_color}'>{bucket_len} replays</font>", - ) + bucket_item.setText(1, f"{bucket_len} replays") + bucket_item.setForeground(1, QtGui.QColor(self.bucket_item_count_color))
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit_tests/themes/test_themeset.py (1)
121-142: Fix typo:builtint→builtin.Line 141 sets
builtint=True, which is a no-op and can mislead future edits/tests.🐛 Proposed fix
- builtint=True, + builtin=True,
♻️ Duplicate comments (1)
src/util/theme.py (1)
57-66: Unthemed detection should key offthemedir, notname.Line 60 checks
self.name is None, but the unthemed Theme is created withname="", so the light stylesheet is never selected on Windows Vista. Useself._themedir is None(or similar) to detect unthemed.🐛 Proposed fix
- if ( - self.name is None # unthemed - and QtWidgets.QApplication.style().name() == "windowsvista" - ): + if ( + self._themedir is None # unthemed + and QtWidgets.QApplication.style().name() == "windowsvista" + ):
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/util/theme.py (1)
285-298: Incorrect return type annotation.The method returns
theme_changed()which is abool, but the annotation says-> None.🐛 Proposed fix
- def _do_setTheme(self, new_theme: Theme) -> None: + def _do_setTheme(self, new_theme: Theme) -> bool:src/fa/maps.py (1)
158-162: Guard empty base-folder usage to avoid resolving maps from CWD.Returning
""makesos.path.join(getBaseMapsFolder(), mapname)resolve to a relative path. Inexport_preview_from_map(Lines 204–210) this can unintentionally scan the current working directory when no game path is set. Please guard the base-folder usage before joining.Example pattern to apply where base-folder joins are used:
base_folder = getBaseMapsFolder() if base_folder and os.path.isdir(os.path.join(base_folder, mapname)): mapdir = os.path.join(base_folder, mapname)
🤖 Fix all issues with AI agents
In `@src/fa/maps.py`:
- Around line 527-555: The cache lookup in get_map uses the original name which
can differ in case from the lowercased keys stored in installed_maps; change the
lookup to use a normalized key (e.g., key = name.lower()) and check
self.installed_maps[key] instead of self.installed_maps[name], then continue to
use dr.name.lower() when inserting (self.installed_maps[dr.name.lower()] =
map_info) so subsequent lookups succeed case-insensitively; ensure you update
any references in get_map that compare or return keys to use the normalized
form.
- Around line 141-145: The view_folder method in mapdetails.py calls
util.showDirInFileBrowser with the result of
maps.folderForMap(self.item_version.folder_name) without checking for None;
update view_folder to first assign the result to a local variable (e.g., folder
= maps.folderForMap(self.item_version.folder_name)) and only call
util.showDirInFileBrowser(folder) if folder is not None to avoid runtime errors
when base maps return None.
In `@src/util/theme.py`:
- Around line 128-136: Update the return type of readstylesheet to reflect that
`@_noneIfNoFile` can return None: change the signature of def readstylesheet(self,
filename: str) -> str to use an optional type (str | None or Optional[str]) so
it matches the decorator behavior; locate the method readstylesheet and the
decorator _noneIfNoFile when making this change.
- Around line 468-473: In find_stylesheet_style_as_dict, guard against lines
without a colon by filtering sheetlines to only non-empty lines that contain ":"
before unpacking, and use line.split(":", 1) to split into name and value (so
values containing ":" are preserved); then apply the existing .strip() and
replace("-", "_") and remove any trailing punctuation as before—this prevents
ValueError from lines like empty trailing newlines or malformed entries.
🧹 Nitpick comments (2)
src/util/theme.py (2)
160-169: Prefer explicitreturn Nonefor clarity.The bare
returnon line 162 implicitly returnsNone. For consistency with the type hint-> str | Noneand readability, use an explicitreturn None.♻️ Suggested fix
def find_stylesheet_attribute(self, section: str, attribute: str) -> str | None: if self.stylesheet is None: - return + return None
171-179: Same as above: prefer explicitreturn None.Line 173 uses a bare
return. For consistency, usereturn None.
Gatsik
force-pushed
the
ui-tweak
branch
2 times, most recently
from
71697fa to
5b60fd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/theme.py (1)
285-298: Fix return type annotation for_do_setTheme.The method returns
theme_changed()(abool) on all paths, but the signature says-> None. Update the annotation to-> bool.
🤖 Fix all issues with AI agents
In `@src/chat/channel_view.py`:
- Around line 380-384: In _sender_name, guard against data.meta.player being
None before calling ChatterFormat.name: when data.line.sender is present but
data.meta.player is None, call ChatterFormat.name(data.line.sender, None)
instead of data.meta.player.clan(); keep the existing html.escape(sender) return
path and preserve the empty-string branch when data.line.sender is None so you
don't raise an AttributeError.
- Around line 272-277: Move the class-level QFontMetrics initialization into the
ChatLineFormatter.__init__ to avoid creating QFontMetrics before a
QGuiApplication exists: remove the class attribute _font_metrics =
QFontMetrics(QFont()) and instead assign self._font_metrics =
QFontMetrics(QFont()) inside __init__ (after calling self._set_theme), so all
usage of _font_metrics continues to reference the instance attribute on
ChatLineFormatter.
In `@src/games/gamepanelwidget.py`:
- Around line 345-353: In set_map_size, guard against non-numeric or placeholder
entries in map_info["map_size"] before calling map(int, ...); replace the direct
map(int, ...) call with a safe parse that tries to convert width and height to
ints (using the values from fa.maps.CachedMapsMetadata.get_map via
map_info["map_size"]), and if either value is missing or not numeric (e.g.,
"-"), set the UI text to "???" via self.ui.mapSizeLabel.setText; ensure the
function still divides numeric values by 51.2 and formats them as before when
parsing succeeds.
In `@tests/unit_tests/themes/test_themeset.py`:
- Around line 136-142: The mock other_t_mock created with
other_t_mock.configure_mock mistakenly sets builtint=True; update this to
builtin=True to reflect the intended attribute name (referencing other_t_mock
and configure_mock), or if other_t_mock is truly unused in this test, remove the
entire mock creation block to keep the test clean.
♻️ Duplicate comments (2)
src/util/theme.py (1)
58-65: Unthemed detection should use the theme directory, notname.Line 60 checks
self.name is None, but_unthemedis created with an empty name, so the light stylesheet branch never triggers. Consider checkingself._themedir is None(orself.themedir) instead.res/themes/Dark Blue/client/client.css (1)
1611-1619: Invalid color keyword:medium orchidshould bemediumorchid.Qt/CSS color names are single tokens; the spaced name is likely ignored.
🛠️ Suggested fix
- friend_mod: medium orchid; - self_mod: medium orchid; + friend_mod: mediumorchid; + self_mod: mediumorchid;Qt stylesheet supported color names "mediumorchid" vs "medium orchid"
🧹 Nitpick comments (1)
src/util/theme.py (1)
422-426: Avoid eagerly evaluating the cached stylesheet during reload.
hasattr(self._theme, "stylesheet")triggers the cached_property getter, doing file I/O only to immediately delete it. Prefer checkingif "stylesheet" in self._theme.__dict__:before deleting.
and also trigger its loading on channel tab init -- it's slow, but it has to be done at some point anyway using QThread doesn't help as with heavy modules, because enumerating audio devices still blocks the whole application QSoundEffects are currently kind of broken in Qt -- they lag and the sound feels incomplete, but we should do our part properly -- load it and let user disable it in the settings
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
res/notification_system/dialog.ui (1)
88-100: Size constraint conflict:minimumSizewidth exceedsmaximumSizewidth.
labelImagehasminimumSizewidth of 80 butmaximumSizewidth of 64. This is contradictory—Qt will honor the minimum, making the maximum ineffective. Consider aligning these values.Suggested fix
Either increase
maximumSizewidth to match or exceed the minimum:<property name="maximumSize"> <size> - <width>64</width> + <width>80</width> <height>16777215</height> </size> </property>Or reduce
minimumSizewidth if 64 is the intended constraint:<property name="minimumSize"> <size> - <width>80</width> + <width>64</width> <height>80</height> </size> </property>src/replays/replayitem.py (1)
265-286: Reset replay state/live_delay whenendTimebecomes available.If a replay transitions from live to finished,
stateremainsPLAYING/UNKNOWNandlive_delaymay stayTrue, so the status keeps showing “playing”. Setstate = FINISHEDand clearlive_delaywhenendTimeis present (and when leaving delay).🐛 Suggested fix
if replay["endTime"] is None: + self.live_delay = False seconds = time.time() - startDt.timestamp() if seconds > 86400: # more than 24 hours self.duration = "" self.state = ReplayItemState.MISSING elif seconds > 7200: # more than 2 hours self.duration = time.strftime('%H:%M:%S', time.gmtime(seconds)) self.state = ReplayItemState.UNKNOWN elif seconds < 300: # less than 5 minutes self.duration = time.strftime('%H:%M:%S', time.gmtime(seconds)) self.state = ReplayItemState.DELAY self.live_delay = True else: self.duration = time.strftime('%H:%M:%S', time.gmtime(seconds)) self.state = ReplayItemState.PLAYING else: endDt = datetime.strptime(replay["endTime"], '%Y-%m-%dT%H:%M:%SZ') # local time endDt = endDt.replace(tzinfo=timezone.utc).astimezone(tz=None) self.duration = time.strftime( '%H:%M:%S', time.gmtime((endDt - startDt).total_seconds()), ) + self.state = ReplayItemState.FINISHED + self.live_delay = False
🤖 Fix all issues with AI agents
In `@res/games/formatters/mod.html`:
- Around line 12-14: The opened <font color="{color}"> tag in the template (the
one wrapping the line with <font size="-1">with</font> <b>{players} /
{mapslots} {playerstring}</b>) is not closed, causing color to bleed; fix it by
adding a corresponding </font> closing tag after the bold segment (i.e., close
the outer <font color="{color}"> just before the existing </td>) so both <font>
elements are properly paired.
In `@res/themes/unthemed/client/client_light.css`:
- Around line 222-233: The QSS uses incomplete font shorthands that may be
ignored; update the selectors QHeaderView::section and
QHeaderView#VerticalHeader::section (and other occurrences flagged) to replace
statements like "font: bold;" or "font: 11px;" with explicit properties such as
font-weight and font-size (e.g., add font-weight: bold; font-size: 11px;) and
remove the broken shorthand; ensure you also fix any other similar occurrences
mentioned in the review (the later blocks around the file) so all font
declarations are explicit.
In `@res/themes/unthemed/client/client.css`:
- Around line 780-781: Replace the invalid CSS color tokens "medium orchid" with
the correct CSS named color "mediumorchid" for the friend_mod and self_mod
declarations so the stylesheet uses a valid color value; locate the friend_mod
and self_mod entries in the unthemed client CSS and update their values
accordingly.
In `@src/notifications/ns_dialog.py`:
- Around line 72-73: The code calls sound_effect(util.THEME) twice creating two
QSoundEffect instances so isLoaded() is checked on one and play() is invoked on
another, which can prevent playback; modify the implementation to create and
cache a single QSoundEffect instance per theme (e.g., a module-level or
class-level dict keyed by util.THEME) and then call isLoaded() and play() on
that same cached instance (reference the sound_effect factory or accessor and
methods isLoaded() and play() to locate where to change).
♻️ Duplicate comments (8)
src/config/version.py (1)
102-102: Missing newline in error message.The error message lacks a trailing newline, causing subsequent shell output to appear on the same line. Line 134 correctly uses
os.linesep.🐛 Suggested fix
- sys.stderr.write(f"Error grabbing git version: {e}") + sys.stderr.write(f"Error grabbing git version: {e}{os.linesep}")src/games/hostgamewidget.py (1)
352-368: Name/type filters still override each other (empty text + toggle changes).When the name filter is empty, you unhide everything (Line 358–360), which ignores the active mod-type filter; and toggling type still bypasses the current name filter because
on_mod_display_type_changeddoesn’t reuse this logic. This recreates the prior “filters override each other” bug.🔧 Suggested fix (keep type filter applied even when text is empty)
def filter_mods_by_name(self, text: str) -> None: lower_text = text.lower() for row in range(self.ui.modList.count()): item = self.ui.modList.item(row) if item is None: continue - if lower_text == "": - item.setHidden(False) - continue mod = self.mods[item.text()] text_matches = lower_text in item.text().lower() if self.ui.modAllRadio.isChecked(): item.setHidden(not text_matches) elif self.ui.modUiRadio.isChecked(): item.setHidden(not text_matches or not mod.ui_only) elif self.ui.modSimRadio.isChecked(): item.setHidden(not text_matches or mod.ui_only)Also consider having
on_mod_display_type_changeddelegate to this method (e.g.,self.filter_mods_by_name(self.ui.modNameFilter.text())) to avoid filter overrides.src/news/_newswidget.py (1)
144-152: Guard againstNone/empty links before opening the browser.
external_link/linkcan beNone; the current empty-string check won’t catch that, soQUrl(None)can be invalid. Use.get(... )+ early return (and optionally validateQUrl) before callingopenUrl().🐛 Suggested fix
def open_news_in_browser(self) -> None: current = self.newsList.currentItem() if current is None: return - if current.newsPost["external_link"] == "": - external_link = current.newsPost["link"] - else: - external_link = current.newsPost["external_link"] - QDesktopServices.openUrl(QUrl(external_link)) + external_link = current.newsPost.get("external_link") or current.newsPost.get("link") + if not external_link: + return + qurl = QUrl(external_link) + if qurl.isValid(): + QDesktopServices.openUrl(qurl)res/themes/Dark Blue/client/client.css (1)
1617-1618: Fix invalid CSS color namemedium orchid.CSS color names are single words;
medium orchidwill be ignored. Usemediumorchidinstead.🐛 Suggested fix
- friend_mod: medium orchid; - self_mod: medium orchid; + friend_mod: mediumorchid; + self_mod: mediumorchid;res/client/client.css (1)
980-1034: Fix remaining::hoverpseudo-elements so hover rules apply.Qt pseudo-states use a single colon. These selectors still use
::hover, so the rules won’t match.🛠️ Suggested fix
-QPushButton#refreshButton:hover, `#nextButton`::hover, `#previousButton`::hover, `#goToPageButton`::hover, -#firstButton:hover, `#lastButton`::hover, `#resetButton`::hover, `#searchPlayerButton`::hover, -#UIButton:hover, `#uploadButton`::hover, `#kickButton`::hover, `#leaveButton`::hover +QPushButton#refreshButton:hover, `#nextButton`:hover, `#previousButton`:hover, `#goToPageButton`:hover, +#firstButton:hover, `#lastButton`:hover, `#resetButton`:hover, `#searchPlayerButton`:hover, +#UIButton:hover, `#uploadButton`:hover, `#kickButton`:hover, `#leaveButton`:hover @@ -QPushButton#searchButton:hover, `#advSearchButton`::hover, `#fafDbButton`::hover, `#spookyDbButton`::hover +QPushButton#searchButton:hover, `#advSearchButton`:hover, `#fafDbButton`:hover, `#spookyDbButton`:hover @@ -QPushButton#RefreshResetButton:hover, `#advResetButton`::hover +QPushButton#RefreshResetButton:hover, `#advResetButton`:hover @@ -QPushButton#saveMapGenSettingsButton:hover, `#resetMapGenSettingsButton`::hover,`#generateMapButton`::hover, +QPushButton#saveMapGenSettingsButton:hover, `#resetMapGenSettingsButton`:hover, `#generateMapButton`:hover, `#hostGameButton`:hover, `#saveHostGameSettingsButton`:hoversrc/util/theme.py (1)
57-65: Usethemedirto detect unthemed for the light stylesheet.
self.namecan be empty (e.g., the unthemed theme is created with""), so the Vista light stylesheet won’t be selected.🛠️ Suggested fix
- if ( - self.name is None # unthemed - and QtWidgets.QApplication.style().name() == "windowsvista" - ): + if ( + self._themedir is None # unthemed + and QtWidgets.QApplication.style().name() == "windowsvista" + ):src/chat/channel_view.py (2)
272-274: QFontMetrics at class scope can run before QApplication.Constructing
QFontMetricsat import time can happen before aQGuiApplicationexists; move it to__init__so it’s created after app startup.
380-384: Guard against missing player metadata.If
data.meta.playercan beNone, Line 383 will raise. Add a None‑safe clan lookup.✅ Proposed guard
def _sender_name(self, data: ChatLineMetadata) -> str: if data.line.sender is None: return "" - sender = ChatterFormat.name(data.line.sender, data.meta.player.clan()) + clan = data.meta.player.clan() if data.meta.player else None + sender = ChatterFormat.name(data.line.sender, clan) return html.escape(sender)
🧹 Nitpick comments (8)
src/news/wpapi.py (2)
22-43: Consider emitting an error signal on processing failure.The exception handler logs the error but silently swallows it. Consumers waiting for
newsDonewon't know that processing failed. Consider either:
- Emitting an error signal (e.g.,
newsError)- Emitting an empty list to indicate failure
- Re-raising if callers should handle it
This is a pre-existing pattern, so flagging as optional.
45-49: Type mismatch acknowledged but consider a cleaner approach.The
# type: ignore[arg-type]is documented, but the underlying issue is thatJsonApiBase.decode_reply()casts todict[str, Any]while this API returns a JSON array (list). This works at runtime becausejson.loads()handles both.For a cleaner solution, consider:
- Adding a list-returning variant to
JsonApiBase(e.g.,get_list())- Or overriding
_decode_and_handlein a subclass for list responsesThe current approach is pragmatic and functional, so this is optional.
src/fa/maps.py (1)
527-553: Case-insensitive cache lookup implemented.The lookup now correctly uses
name.lower()at line 529, addressing the previous review feedback. The method properly falls through to returnNoneimplicitly when no map is found.Consider extracting the shared folder-scanning logic between
get_mapandget_installed_mapsinto a helper to reduce duplication, though this is a minor refactor.src/replays/replayitem.py (1)
34-52: Theme-derived colors are captured once; they won’t update on theme switch.
ICON_*and the cachedreplay_item_colors()are computed at import time, so runtime theme changes won’t refresh replay colors. Consider fetching fromutil.THEMEinpaint()or clearing the cache onstylesheets_reloaded.Also applies to: 165-167
src/news/newsitem.py (1)
69-71: Formatter/colors cached at class load; won’t update on theme change.
FORMATTERandCOLORSare class attributes computed once, so theme switching won’t refresh existing items unless they’re recreated. Consider resolving these in__init__or refreshing onstylesheets_reloaded.src/util/theme.py (1)
422-427: Avoid computing cached stylesheets just to clear them.
hasattr(self._theme, "stylesheet")triggers the cached property (and file read) before deletion. Use__dict__to clear without loading.♻️ Suggested refactor
- if hasattr(self._theme, "stylesheet"): - del self._theme.stylesheet - if hasattr(self._default_theme, "stylesheet"): - del self._default_theme.stylesheet + self._theme.__dict__.pop("stylesheet", None) + self._default_theme.__dict__.pop("stylesheet", None)src/chat/channel_view.py (2)
163-167: Defensive guard for unset topics.If
Channel.topiccan beNone, Line 167 would callirc_escape(None)and raise. Consider clearing on falsy and (optionally) widening the type hint. Please verify whethertopiccan be null/unset.🛠️ Suggested guard
- def _set_topic(self, topic: str) -> None: - if topic == "": + def _set_topic(self, topic: str) -> None: + if not topic: self._widget.clear_topic() else: self._widget.set_topic(irc_escape(topic))
329-333: Escape the mention token before replace.
textis already HTML‑escaped, so names with&or<won’t matchreplace(me, …). Escaping the token keeps highlighting consistent for such names.♻️ Suggested tweak
def _wrap_me(self, text: str, me: str | None) -> str: if me: - return text.replace(me, f"<span class=\"my_mention\">{me}</span>") + safe_me = html.escape(me) + return text.replace( + safe_me, + f"<span class=\"my_mention\">{safe_me}</span>", + ) else: return text
| <font color="{color}"> | ||
| <font size="-1">with</font> <b>{players} / {mapslots} {playerstring}</b> | ||
| </td> |
There was a problem hiding this comment.
Unclosed <font> tag.
The <font color="{color}"> opened on line 12 is never closed before the </td> on line 14. This will cause the color to bleed into subsequent elements.
🛠️ Proposed fix
<td>
<font color="{color}">
<font size="-1">with</font> <b>{players} / {mapslots} {playerstring}</b>
+ </font>
</td>📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <font color="{color}"> | |
| <font size="-1">with</font> <b>{players} / {mapslots} {playerstring}</b> | |
| </td> | |
| <font color="{color}"> | |
| <font size="-1">with</font> <b>{players} / {mapslots} {playerstring}</b> | |
| </font> | |
| </td> |
🤖 Prompt for AI Agents
In `@res/games/formatters/mod.html` around lines 12 - 14, The opened <font
color="{color}"> tag in the template (the one wrapping the line with <font
size="-1">with</font> <b>{players} / {mapslots} {playerstring}</b>) is not
closed, causing color to bleed; fix it by adding a corresponding </font> closing
tag after the bold segment (i.e., close the outer <font color="{color}"> just
before the existing </td>) so both <font> elements are properly paired.
| QHeaderView::section | ||
| { | ||
| font: bold; | ||
| border-left: 1 solid; | ||
|
|
||
| } | ||
|
|
||
| QHeaderView#VerticalHeader::section { | ||
| border: none; | ||
| font: normal; | ||
| min-width: 50px; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the content at lines 222-233
if [ -f "res/themes/unthemed/client/client_light.css" ]; then
echo "=== Lines 222-233 ==="
sed -n '222,233p' res/themes/unthemed/client/client_light.css
echo ""
echo "=== Lines 385-407 ==="
sed -n '385,407p' res/themes/unthemed/client/client_light.css
echo ""
echo "=== Lines 484-487 ==="
sed -n '484,487p' res/themes/unthemed/client/client_light.css
else
echo "File not found"
fiRepository: FAForever/client
Length of output: 771
🏁 Script executed:
# Also search for all font: declarations in the file to see the pattern
echo "=== All font: declarations in the file ==="
rg "^\s*font:" res/themes/unthemed/client/client_light.css -n -A 0Repository: FAForever/client
Length of output: 234
🌐 Web query:
Qt QSS font shorthand property size family requirement
💡 Result:
Short answer: use the size before the family (family comes last). Qt's QSS font shorthand follows the form shown in the docs (e.g. font: bold italic large "Times New Roman"), and font-size must use pt or px. If the family name contains spaces quote it; when multiple family names are listed Qt will try the first and fall back to system fonts. [1][2][3]
Sources:
- Qt Style Sheets Reference (example and description of
fontshorthand). [1] - Qt for Python stylesheet reference (same shorthand example;
font-sizeunits). [2] - Qt docs on font family behaviour (first family used / fallback). [3]
Replace incomplete font: shorthands with explicit properties.
Qt QSS font shorthand requires both size and family (e.g., font: bold 12px Arial). Declarations like font: bold;, font: 11px;, and font: bold 12px; are incomplete and may be ignored. Use font-weight and font-size properties instead.
♻️ Proposed fix
QHeaderView::section
{
- font: bold;
+ font-weight: bold;
border-left: 1 solid;
}
QHeaderView#VerticalHeader::section {
border: none;
- font: normal;
+ font-weight: normal;
min-width: 50px;
}
QPushButton#searchButton, `#advSearchButton`, `#fafDbButton`, `#spookyDbButton`
{
border-radius: 5px;
border: 2px solid;
- font: bold 12px;
+ font-weight: bold;
+ font-size: 12px;
min-height: 20px;
}
QPushButton#RefreshResetButton, `#advResetButton`
{
border-radius: 5px;
border: 1px solid;
- font: 11px;
+ font-size: 11px;
min-height: 20px;
}
QPushButton#generateMapButton, `#hostGameButton`
{
border-radius: 2px;
border: 0.5px solid;
- font: bold 14px;
+ font-weight: bold;
+ font-size: 14px;
min-height: 30px;
}
QLabel#vaultListItemTitle
{
- font: bold;
+ font-weight: bold;
}Also applies to: 385-407, 484-487
🧰 Tools
🪛 Biome (2.1.2)
[error] 222-222: Unexpected unknown pseudo-elements: section
See MDN web docs for more details.
Use a known pseudo-elements instead, such as:
(lint/correctness/noUnknownPseudoElement)
[error] 228-229: Unknown type selector is not allowed.
See MDN web docs for more details.
Consider replacing the unknown type selector with valid one.
(lint/correctness/noUnknownTypeSelector)
[error] 229-229: Unexpected unknown pseudo-elements: section
See MDN web docs for more details.
Use a known pseudo-elements instead, such as:
(lint/correctness/noUnknownPseudoElement)
🤖 Prompt for AI Agents
In `@res/themes/unthemed/client/client_light.css` around lines 222 - 233, The QSS
uses incomplete font shorthands that may be ignored; update the selectors
QHeaderView::section and QHeaderView#VerticalHeader::section (and other
occurrences flagged) to replace statements like "font: bold;" or "font: 11px;"
with explicit properties such as font-weight and font-size (e.g., add
font-weight: bold; font-size: 11px;) and remove the broken shorthand; ensure you
also fix any other similar occurrences mentioned in the review (the later blocks
around the file) so all font declarations are explicit.
| friend_mod: medium orchid; | ||
| self_mod: medium orchid; |
There was a problem hiding this comment.
Invalid CSS color name with space.
medium orchid is not a valid CSS color name. CSS named colors don't contain spaces. This should be mediumorchid.
✏️ Suggested fix
- friend_mod: medium orchid;
- self_mod: medium orchid;
+ friend_mod: mediumorchid;
+ self_mod: mediumorchid;📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| friend_mod: medium orchid; | |
| self_mod: medium orchid; | |
| friend_mod: mediumorchid; | |
| self_mod: mediumorchid; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 780-780: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
[error] 781-781: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🤖 Prompt for AI Agents
In `@res/themes/unthemed/client/client.css` around lines 780 - 781, Replace the
invalid CSS color tokens "medium orchid" with the correct CSS named color
"mediumorchid" for the friend_mod and self_mod declarations so the stylesheet
uses a valid color value; locate the friend_mod and self_mod entries in the
unthemed client CSS and update their values accordingly.
| if sound and sound_effect(util.THEME).isLoaded(): | ||
| sound_effect(util.THEME).play() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the notification file
find . -name "ns_dialog.py" -type fRepository: FAForever/client
Length of output: 92
🏁 Script executed:
# Also search for sound_effect function definition
rg "def sound_effect" -A 10Repository: FAForever/client
Length of output: 309
🏁 Script executed:
# Search for imports in the notification file to understand the context
rg "from|import" src/notifications/ns_dialog.py | head -20Repository: FAForever/client
Length of output: 210
🏁 Script executed:
# Let's examine the actual code at lines 72-73 in ns_dialog.py
cat -n src/notifications/ns_dialog.py | sed -n '65,85p'Repository: FAForever/client
Length of output: 921
🏁 Script executed:
# Let's also look at the __init__ method to understand the class structure
cat -n src/notifications/ns_dialog.py | sed -n '1,100p'Repository: FAForever/client
Length of output: 3745
Reuse a single QSoundEffect instance; current code can stay silent.
sound_effect(util.THEME) constructs a new QSoundEffect on each call, so isLoaded() is checked on one instance while play() is invoked on another fresh instance. This typically results in no playback. Cache and reuse the same instance.
🐛 Suggested fix
class NotificationDialog(FormClass, BaseClass):
def __init__(self, client, settings, *args, **kwargs):
BaseClass.__init__(self, *args, **kwargs)
...
self.setStyleSheet(util.THEME.stylesheet)
+ self._event_sound = sound_effect(util.THEME)
...
def newEvent(...):
...
- if sound and sound_effect(util.THEME).isLoaded():
- sound_effect(util.THEME).play()
+ if sound and self._event_sound.isLoaded():
+ self._event_sound.play()🤖 Prompt for AI Agents
In `@src/notifications/ns_dialog.py` around lines 72 - 73, The code calls
sound_effect(util.THEME) twice creating two QSoundEffect instances so isLoaded()
is checked on one and play() is invoked on another, which can prevent playback;
modify the implementation to create and cache a single QSoundEffect instance per
theme (e.g., a module-level or class-level dict keyed by util.THEME) and then
call isLoaded() and play() on that same cached instance (reference the
sound_effect factory or accessor and methods isLoaded() and play() to locate
where to change).
1 participant
Summary by CodeRabbit
New Features
Refactor
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.