Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Information Widget: code clean up and slight facelift #2119

Merged
merged 5 commits into from Dec 15, 2016

Conversation

@declension
Copy link
Member

@declension declension commented Nov 25, 2016

  • For OneSong, lyrics and bookmarks are optional
  • Start cleaning up and extracting common code in advance of proper modularisation
  • Use more modern features where available (defaultdict, comprehensions, if expressions) to shorten code
  • Lose the bold and underline, go for a lighter approach in keeping with other UIs / web
  • Italicise tracks / album names where relevant, to indicate these are (chosen) names not values.
  • Add comments (Fixes #1558) website (mainly for Soundcloud) into a new section.
  • Use regular table layout more for neatness
  • More is possible / changeable, this is just to make sure people are happyish 馃槃
 * For OneSong, lyrics and bookmarks are optional
 * Start cleaning up and extracting common code in advance of proper modularisation
 * Use more modern features where available (`defaultdict`, comprehensions, `if` expressions) to shorten code
 * Lose the bold and underline, go for a lighter approach in keeping with other UIs / web
 * Italicise tracks / album names where relevant, to indicate these are (chosen) names not values.
 * Add `comments` (Fixes #1558)  `website` (mainly for Soundcloud) into a new section.
 * Use regular table layout more for neatness
 * More is possible / changeable, this is just to make sure people are happyish 馃槃


def get_colors(widget):
context = widget.get_style_context()

This comment has been minimized.

@declension

declension Nov 26, 2016
Author Member

Oh, I saw that code originally... applied a few inlinings (assuming referential transparency etc, I guess) and it seemed redundant. Seemed to work without it, so I left the simplified version.

Guess it does something magic on state saving?

This comment has been minimized.

@lazka

lazka Nov 26, 2016
Member

Yeah, it's not obvious and only a problem since gtk 3.20+. The problem is that the getters invalidate things for gtk and when called from the wrong code path you get in an invalidation loop. (new-state -> update css/drawing -> new-state ...). The save/restore part prevents gtk from noticing.

This comment has been minimized.

@declension

declension Nov 26, 2016
Author Member

Madness! Thanks


def get_colors(widget):
context = widget.get_style_context()
bg_color = context.get_background_color(Gtk.StateFlags.NORMAL)

This comment has been minimized.

@lazka

lazka Nov 25, 2016
Member

get_background_color no longer works with newer gtk+ (it only returns the css solid color part, which might not be used when rendering afaik)

@lazka
Copy link
Member

@lazka lazka commented Nov 26, 2016

(btw: it's always a good idea to test theme specific things with the Adwaita theme (also the dark variant), check out the theme switcher plugin)

@declension
Copy link
Member Author

@declension declension commented Nov 26, 2016

Yep, might just ditch that colours thing. Wanted to avoid using bold (only) for the frames but increasingly I'm believing that Gtk.Expanders might be the nicest experience (especially when used in the forthcoming information widget plugin maybe). Better still if they could made to save their state too, one day at least (like the queue I guess)

declension added 2 commits Nov 27, 2016
 * Grouped views have more People information now.
 * Use italics better (notably *not* when we're indicating missing tag values)
 * Remove use of custom colours
 * Extract a bit more common code out.
 * Tighten `Tinformation` assertions
@lazka
Copy link
Member

@lazka lazka commented Dec 15, 2016

lgtm

@declension
Copy link
Member Author

@declension declension commented Dec 15, 2016

Thanks

@declension declension merged commit ce893d8 into master Dec 15, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@declension declension deleted the information-window-cleanup branch Dec 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants