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 <[email protected]> wrote: > On 7 October 2014 20:32, Mark R. Pariente <[email protected]> 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
From b696ea199e654a7dfe5d123664596e9b6f43114f Mon Sep 17 00:00:00 2001 From: Mark Pariente <[email protected]> Date: Wed, 8 Oct 2014 10:27:39 -0700 Subject: [PATCH 1/2] Shuffle around UI as asked by Kai - Rename preference to 'Syntax highlight color scheme' - Move preference under the syntax highlight checkbox --- data/ui/preferences.ui | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/data/ui/preferences.ui b/data/ui/preferences.ui index b246af7..2f5caf0 100644 --- a/data/ui/preferences.ui +++ b/data/ui/preferences.ui @@ -465,40 +465,37 @@ </packing> </child> <child> + <object class="GtkCheckButton" id="checkbutton_use_syntax_highlighting"> + <property name="label" translatable="yes">Use s_yntax highlighting</property> + <property name="visible">True</property> + <property name="can_focus">True</property> + <property name="receives_default">False</property> + <property name="use_underline">True</property> + <property name="xalign">0</property> + <property name="draw_indicator">True</property> + </object> + <packing> + <property name="expand">False</property> + <property name="fill">False</property> + <property name="position">7</property> + </packing> + </child> + <child> <object class="GtkHBox" id="hbox11"> <property name="visible">True</property> <property name="can_focus">False</property> <property name="spacing">6</property> <child> - <object class="GtkCheckButton" id="checkbutton_use_syntax_highlighting"> - <property name="label" translatable="yes">Use s_yntax highlighting</property> - <property name="visible">True</property> - <property name="can_focus">True</property> - <property name="receives_default">False</property> - <property name="use_underline">True</property> - <property name="xalign">0</property> - <property name="draw_indicator">True</property> - </object> - <packing> - <property name="expand">False</property> - <property name="fill">False</property> - <property name="position">0</property> - </packing> - </child> - <child> <object class="GtkLabel" id="label16"> <property name="visible">True</property> <property name="can_focus">False</property> - <property name="label" translatable="yes">Color Scheme:</property> - <attributes> - <attribute name="weight" value="bold"/> - </attributes> + <property name="label" translatable="yes">Syntax highlighting color scheme:</property> </object> <packing> <property name="expand">True</property> <property name="fill">True</property> <property name="padding">8</property> - <property name="position">1</property> + <property name="position">0</property> </packing> </child> <child> @@ -517,14 +514,14 @@ <packing> <property name="expand">True</property> <property name="fill">True</property> - <property name="position">2</property> + <property name="position">1</property> </packing> </child> </object> <packing> <property name="expand">False</property> <property name="fill">False</property> - <property name="position">7</property> + <property name="position">8</property> </packing> </child> </object> -- 2.1.2
From 5748e674150b8396aa0e61d16ca99e3287c71496 Mon Sep 17 00:00:00 2001 From: Mark Pariente <[email protected]> Date: Wed, 8 Oct 2014 10:43:29 -0700 Subject: [PATCH 2/2] Move property handling to settings.py As requested by Kai, removed style-scheme property handling from filediff.py and moved it to meld.settings. Now meld.buffer just listens to the changed signal of meld.settings and just changes scheme as a response. Also moved scheme lookup to meld.settings. --- meld/filediff.py | 3 --- meld/meldbuffer.py | 12 ++++++------ meld/settings.py | 9 ++++++++- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/meld/filediff.py b/meld/filediff.py index 5e1f308..5ebe330 100644 --- a/meld/filediff.py +++ b/meld/filediff.py @@ -850,9 +850,6 @@ 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) def check_save_modified(self, label=None): response = Gtk.ResponseType.OK diff --git a/meld/meldbuffer.py b/meld/meldbuffer.py index d3fe665..7599eee 100644 --- a/meld/meldbuffer.py +++ b/meld/meldbuffer.py @@ -24,7 +24,7 @@ from gi.repository import GObject from gi.repository import GtkSource from meld.conf import _ -from meld.settings import bind_settings,settings +from meld.settings import bind_settings,meldsettings from meld.util.compat import text_type @@ -41,12 +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')) + meldsettings.connect('changed', self.on_setting_changed) + GtkSource.Buffer.set_style_scheme(self, meldsettings.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) + def on_setting_changed(self, meldsettings, key): + if key == 'style-scheme': + GtkSource.Buffer.set_style_scheme(self, meldsettings.style_scheme) def do_begin_user_action(self, *args): self.user_action_count += 1 diff --git a/meld/settings.py b/meld/settings.py index d147e31..c651bca 100644 --- a/meld/settings.py +++ b/meld/settings.py @@ -19,6 +19,7 @@ from gi.repository import Gio from gi.repository import GLib from gi.repository import GObject from gi.repository import Pango +from gi.repository import GtkSource import meld.conf import meld.filters @@ -41,6 +42,7 @@ class MeldSettings(GObject.GObject): self.on_setting_changed(settings, 'filename-filters') self.on_setting_changed(settings, 'text-filters') self.on_setting_changed(settings, 'use-system-font') + self.style_scheme = self._style_scheme_from_gsettings() settings.connect('changed', self.on_setting_changed) def on_setting_changed(self, settings, key): @@ -56,9 +58,14 @@ class MeldSettings(GObject.GObject): 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.style_scheme = self._style_scheme_from_gsettings() self.emit('changed', 'style-scheme') + def _style_scheme_from_gsettings(self): + manager = GtkSource.StyleSchemeManager.get_default() + return GtkSource.StyleSchemeManager.get_scheme(manager, + settings.get_string('style-scheme')) + def _filters_from_gsetting(self, key, filt_type): filter_params = settings.get_value(key) filters = [ -- 2.1.2
_______________________________________________ meld-list mailing list [email protected] https://mail.gnome.org/mailman/listinfo/meld-list
