Kai, thanks for your detailed review and helpful suggestions. I have implemented your requests as best as I could - I'm attaching two patches on top of my original patch + your patch. Please let me know how it looks. I've also updated the github pull request at: https://github.com/GNOME/meld/pull/2 with these patches. Best, --Mark On Tue, Oct 7, 2014 at 1:28 PM, Kai Willadsen <kai willadsen gmail com> wrote:
On 7 October 2014 20:32, Mark R. Pariente <markpariente gmail com> wrote:This commit adds a "color scheme" entry to preferences which controls the GtkSourceView scheme-style property to allow the user toggle between different color schemes installed in the system. This is particularly relevant for syntax highlighting since the default scheme (classic) is not great in all circumstances such as dark theme usage. In general this is heavily user preference driven so this commit allows users to select a scheme they like.Awesome, thanks! While I have some reservations about this feature, it's definitely something that there have been requests for, so it'll be great to get it in. The reason I'm worried about doing this is that of the five GtkSourceView schemes I have installed, two are basically unreadable with our colours, and we don't (and I think can't) change those with a GtkSourceView theme. So I'm worried that we're sending a signal that we're supporting something that we really can't. On the UI front, I think I'd prefer to have the scheme combo box below the current syntax highlighting preference, in the same way that we do with other preferences. It also needs to be labelled 'Syntax highlighting color scheme' or something, to try and indicate that this *won't* change the diff highlight colours. I've attached a patch on top of yours to make the preferences combo box use our existing GSettingsStringComboBox helper, and move some of the UI creation back into the .ui file.diff --git a/meld/filediff.py b/meld/filediff.py index 5ebe330..5e1f308 100644 --- a/meld/filediff.py +++ b/meld/filediff.py @@ -850,6 +850,9 @@ class FileDiff(melddoc.MeldDoc, gnomeglade.Component): def on_setting_changed(self, settings, key): if key == 'font': self.load_font() + elif key == 'style-scheme': + for i in range(3): + self.textview[i].get_buffer().change_style_scheme(meldsettings.style_scheme)FileDiff shouldn't really need to handle this logic at all. I think it would be neater to have it all in MeldBuffer, where you have most of the relevant stuff already (see below). <snip>@@ -41,6 +41,12 @@ class MeldBuffer(GtkSource.Buffer): bind_settings(self) self.data = MeldBufferData(filename) self.user_action_count = 0 + self.change_style_scheme(settings.get_string('style-scheme')) + + def change_style_scheme(self, scheme): + manager = GtkSource.StyleSchemeManager.get_default() + style = GtkSource.StyleSchemeManager.get_scheme(manager, scheme) + GtkSource.Buffer.set_style_scheme(self, style)I think this would be nicer if we used MeldSettings (which is basically a settings adaptor class) to update the actual scheme for us, and just make MeldBuffer connect to the MeldSettings changed signal. That way, almost all of the settings handling logic ends up in meld.settings. <snip>--- a/meld/settings.py +++ b/meld/settings.py @@ -55,6 +55,9 @@ class MeldSettings(GObject.GObject): elif key in ('use-system-font', 'custom-font'): self.font = self._current_font_from_gsetting() self.emit('changed', 'font') + elif key in ('style-scheme'): + self.style_scheme = settings.get_string('style-scheme') + self.emit('changed', 'style-scheme')So you've set self.style_scheme here, which is cool, but isn't actually used. We *should* set self.style_scheme to just be the actual scheme (i.e., do the StyleSchemeManager stuff there) and then retrieve that in MeldBuffer. For a bit of context, things handled by MeldSettings should (I think) all be g_settings_bind_with_mapping() calls, but that isn't supported by our required PyGObject version. Again, thanks for this. I'm looking forward to getting it in. cheers, Kai
Attachment:
0001-Shuffle-around-UI-as-asked-by-Kai.patch
Description: Text Data
Attachment:
0002-Move-property-handling-to-settings.py.patch
Description: Text Data