-
Notifications
You must be signed in to change notification settings - Fork 433
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhanced version of the Filter gramplet #1721
Conversation
Very nice! You have one of the few functional aborts in Gramps for your Filter Params test feature. I cannot count the number of times that the Filter progress dialog has appeared and I have wished for an abort. (Usually because I had set a parameter incorrectly.) Could that test abort routine be made compatible with the Filter Progress dialog? It would probably have to abort to "unfiltered"... would that be intuitive enough? |
Could that test abort routine be made compatible with the Filter Progress dialog?
Not easily.
la 11. toukok. 2024 klo 21.07 Emyoulation or BAMaustin (
***@***.***) kirjoitti:
… Very nice!
You have one of the few functional aborts in Gramps for your Filter Params
test feature.
I cannot count the number of times that the Filter progress dialog has
appeared and I have wished for an abort. (Usually because I had set a
parameter incorrectly.)
Could that test abort routine be made compatible with the Filter Progress
dialog?
It would probably have to abort to "unfiltered"... would that intuitive
enough? I
—
Reply to this email directly, view it on GitHub
<#1721 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIYOXTFFGNKELT2TSWX74TZBZM7RAVCNFSM6AAAAABHSDFO3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBVHE3TQNJQHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Understood and thanks for looking... and another 'thanks' for not being aggravated by the extra ask. Looks like the PR test fails are formatting compliance. I ran into similar fails for the last PR that I tried. And they would have had the same fails for the module before my changes. Maybe there's a way to start re-submit the original modules for a compliance check with the new requirements. Although the new proposal (for Type Hints and static type checking) might make that necessary again in the near future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I just made a few suggestions.
To get rid of the formatting errors, run black on the files you changed.
rule.list[2] = str(rule.list[2]) | ||
new_rules.append(rule) | ||
the_filter.flist = new_rules | ||
comment = "Created by Filter gramplet on " + time.strftime("%Y-%m-%d", time.localtime(time.time())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string looks like it should be translated. Should we use the current locale settings for the date format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit to fix this. I think we can leave the date in ISO format.
if isinstance(rule, gramps.gen.filters.rules.place.WithinArea): | ||
if rule.list[0] is None: # No active place | ||
msg = _("You should select a place when using the 'Within' rule") | ||
self.msg_label.set_markup("<span color='red'>" + msg + "</span>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to add the css error class to the widget style context rather than hard-coding the colour "red". For example you could use:
self.msg_label.set_text(msg)
self.msg_label.get_style_context().add_class("error")
This will use the error colour defined in the theme. This is useful for visually impaired users and non-European cultures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand how css and themes work in Gramps. If I add the "error" class then the label still doesn't change color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I've pushed a commit to your branch which works for me. The errors are red, but the elapsed time is black.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make the css style work if I manually make changes in the file ~/.config/gtk-3.0/gtk.css. But that is probably not the correct the way to do this, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, it seems that we are doing changes simultaneously. Did I mess things up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What operating system and theme are you using? No changes to the css should be necessary.
When you run Gramps in the master branch, what colour is the version number in the status bar? That is set using the "destructive-action" class.
You can either use my changes or update the code yourself.
self.filterdb = gramps.gen.filters.CustomFilters | ||
the_filter = self.get_filter() | ||
if the_filter is None: | ||
self.msg_label.set_markup("<span color='red'>" + _("Supply at least one value") + "</span>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
@@ -71,4 +72,5 @@ | |||
HasTag, | |||
MatchesPlaceFilter, | |||
HasDayOfWeek, | |||
HasEventBase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is HasEventBase
and HasSourceBase
related to this PR? I can't see them referenced anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'Define filter' feature does not work without it.
Event filtering uses the rule class HasEventBase and 'Define filter' will store that class name in custom_filters.xml. For example:
<object type="Event">
<filter name="test" function="and" comment="Created by Filter gramplet on 2024-05-12">
<rule class="HasEventBase" use_regex="False" use_case="False">
...
</rule>
</object>
However, when custom_filters.xml is read and interpreted then the corresponding rule is silently ignored because the class is not imported in gramps.gen.filters.rules.event.__init__.py
. There is only this message on the console:
ERROR: Filter rule 'HasEventBase' in filter 'test' not found!
My solution is to import the class in the __init__.py
file. At first i thought it also needed to be added in 'editor_rule_list' but that does not seem to be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any warnings or errors if I comment these lines out.
The HasEventBase
is used as the "Events with parameters" rule. It works if I click the 'Define filter' button in the sidebar, or if I invoke the filter editor from the menu.
Kari, Could you add your 2024 copyright to the list of contributors for these 5 files? It isn't just about taking credit. It also agrees to the terms of the licensing for your contribution. And it also builds confidence by showing that the modules are continuing to evolve. Most of these show 2010 as the last touch. Cross-link to Discourse forum discussion: MantisBT request 0013289: Roll Filter+ gramplet plugins into the core (built-in) plugins for 5.3 |
I am using Linux Mint and I don't know what theme I am using! How to check
that?
su 12. toukok. 2024 klo 20.56 Nick Hall ***@***.***)
kirjoitti:
… ***@***.**** commented on this pull request.
------------------------------
In gramps/gui/filters/sidebar/_sidebarfilter.py
<#1721 (comment)>
:
> @@ -316,3 +363,47 @@ def set_filters_to_name(self, filter_name):
self.generic.set_active_iter(iter)
break
iter = liststore.iter_next(iter)
+
+ def filter_is_ok(self):
+ the_filter = self.get_filter()
+ if the_filter is None:
+ return True
+ for rule in the_filter.get_rules():
+ if isinstance(rule, gramps.gen.filters.rules.place.WithinArea):
+ if rule.list[0] is None: # No active place
+ msg = _("You should select a place when using the 'Within' rule")
+ self.msg_label.set_markup("<span color='red'>" + msg + "</span>")
What operating system and theme are you using? No changes to the css
should be necessary.
When you run Gramps in the *master* branch, what colour is the version
number in the status bar? That is set using the "destructive-action" class.
You can either use my changes or update the code yourself.
—
Reply to this email directly, view it on GitHub
<#1721 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIYOXQTJLMKW5ZYEC4IMXTZB6UM7AVCNFSM6AAAAABHSDFO3WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJRGQZDIMBSHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Yes, it is sufficient to have HasEventBase imported in the module. I
noticed that also.
su 12. toukok. 2024 klo 20.59 Nick Hall ***@***.***)
kirjoitti:
… ***@***.**** commented on this pull request.
------------------------------
In gramps/gen/filters/rules/event/__init__.py
<#1721 (comment)>
:
> @@ -71,4 +72,5 @@
HasTag,
MatchesPlaceFilter,
HasDayOfWeek,
+ HasEventBase,
I don't see any warnings or errors if I comment these lines out.
The HasEventBase is used as the "Events with parameters" rule. It works
if I click the 'Define filter' button in the sidebar, or if I invoke the
filter editor from the menu.
—
Reply to this email directly, view it on GitHub
<#1721 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIYOXSQT6XQMQEHN6URQ7TZB6UZTAVCNFSM6AAAAABHSDFO3WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJRGQZDIMZWHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
From the command line, start Gramps with:
Then, in the debugger, look in the "Visual" tab. Note: The Adwaita error colour is (204, 0, 0). The colour "red" is (255, 0, 0). |
GTK Theme = Mint-Y
su 12. toukok. 2024 klo 23.14 Nick Hall ***@***.***)
kirjoitti:
… I am using Linux Mint and I don't know what theme I am using! How to check
that?
From the command line, start Gramps with:
GTK_DEBUG=interactive python Gramps.py
Then, in the debugger, look in the "Visual" tab.
—
Reply to this email directly, view it on GitHub
<#1721 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIYOXS7DDKJJ35MOYQR4GLZB7ETLAVCNFSM6AAAAABHSDFO3WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWGM3DEOBUGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The version number is red.
If I add the "destructive-action" class to the Reset button then it turns
red. But using it on the label does not work. The "error" class does not
work for the button either.
su 12. toukok. 2024 klo 20.56 Nick Hall ***@***.***)
kirjoitti:
… ***@***.**** commented on this pull request.
------------------------------
In gramps/gui/filters/sidebar/_sidebarfilter.py
<#1721 (comment)>
:
> @@ -316,3 +363,47 @@ def set_filters_to_name(self, filter_name):
self.generic.set_active_iter(iter)
break
iter = liststore.iter_next(iter)
+
+ def filter_is_ok(self):
+ the_filter = self.get_filter()
+ if the_filter is None:
+ return True
+ for rule in the_filter.get_rules():
+ if isinstance(rule, gramps.gen.filters.rules.place.WithinArea):
+ if rule.list[0] is None: # No active place
+ msg = _("You should select a place when using the 'Within' rule")
+ self.msg_label.set_markup("<span color='red'>" + msg + "</span>")
What operating system and theme are you using? No changes to the css
should be necessary.
When you run Gramps in the *master* branch, what colour is the version
number in the status bar? That is set using the "destructive-action" class.
You can either use my changes or update the code yourself.
—
Reply to this email directly, view it on GitHub
<#1721 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIYOXQTJLMKW5ZYEC4IMXTZB6UM7AVCNFSM6AAAAABHSDFO3WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJRGQZDIMBSHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The Mint-Y theme doesn't seem to define the "error" class for labels, although the Mint-X theme does. The "destructive-action" class is only used by buttons. I suggest that we add the following line to our label.error {color: @error_color} This will let us use classes as recommended. I expect that the @error_color will be used by themes anyway, so overriding it in this way shouldn't cause a problem. |
Have you tried the GTK+ Inspector tool? It was very enlightening when exploring what CSS was applied to each element of the Gramps GUI. For Linux only. Wish there were Windows and macOS versions. |
What happens if you try to start it in Windows? |
I only found GTK+ Inspector in the Gnome project. Haven't found info about installation for Windows or macOS |
The inspector is part of Gtk. Just start Gramps from the command line with e.g.
There are also two keyboard shortcuts, |
I am travelling this week and can't work on this. Please proceed with the
PR if you wish. Anyway, I don't know what I should do next.
la 11. toukok. 2024 klo 23.11 Nick Hall ***@***.***)
kirjoitti:
… ***@***.**** requested changes on this pull request.
This looks good. I just made a few suggestions.
To get rid of the formatting errors, run black
<https://black.readthedocs.io/en/stable/index.html> on the files you
changed.
------------------------------
In gramps/gui/filters/sidebar/_citationsidebarfilter.py
<#1721 (comment)>
:
> @@ -78,7 +78,7 @@ def __init__(self, dbstate, uistate, clicked):
for conf_value in sorted(conf_strings.keys()):
model.append((_(conf_strings[conf_value]),))
self.filter_conf.set_model(model)
- self.filter_conf.set_active(Citation.CONF_NORMAL)
+ self.filter_conf.set_active(Citation.CONF_VERY_LOW)
This looks like a duplicate of PR #1701
<#1701>.
------------------------------
In gramps/gui/filters/sidebar/_sidebarfilter.py
<#1721 (comment)>
:
> + if the_filter is None:
+ self.msg_label.set_markup("<span color='red'>" + _("Supply at least one value") + "</span>")
+ return
+
+ if not self.filter_is_ok():
+ return
+ # fix some rules:
+ new_rules = []
+ for rule in the_filter.get_rules():
+ # The Place rule WithinArea might have numeric values while custom_filters.xml only accepts strings.
+ if isinstance(rule, gramps.gen.filters.rules.place.WithinArea):
+ rule.list[1] = str(rule.list[1])
+ rule.list[2] = str(rule.list[2])
+ new_rules.append(rule)
+ the_filter.flist = new_rules
+ comment = "Created by Filter gramplet on " + time.strftime("%Y-%m-%d", time.localtime(time.time()))
This string looks like it should be translated. Should we use the current
locale settings for the date format?
------------------------------
In gramps/gui/filters/sidebar/_sidebarfilter.py
<#1721 (comment)>
:
> @@ -316,3 +363,47 @@ def set_filters_to_name(self, filter_name):
self.generic.set_active_iter(iter)
break
iter = liststore.iter_next(iter)
+
+ def filter_is_ok(self):
+ the_filter = self.get_filter()
+ if the_filter is None:
+ return True
+ for rule in the_filter.get_rules():
+ if isinstance(rule, gramps.gen.filters.rules.place.WithinArea):
+ if rule.list[0] is None: # No active place
+ msg = _("You should select a place when using the 'Within' rule")
+ self.msg_label.set_markup("<span color='red'>" + msg + "</span>")
It is better to add the css error class to the widget style context rather
than hard-coding the colour "red". For example you could use:
self.msg_label.set_text(msg)
self.msg_label.get_style_context().add_class("error")
This will use the error colour defined in the theme. This is useful for
visually impaired users and non-European cultures.
------------------------------
In gramps/gui/filters/sidebar/_sidebarfilter.py
<#1721 (comment)>
:
> + the_filter = self.get_filter()
+ if the_filter is None:
+ return True
+ for rule in the_filter.get_rules():
+ if isinstance(rule, gramps.gen.filters.rules.place.WithinArea):
+ if rule.list[0] is None: # No active place
+ msg = _("You should select a place when using the 'Within' rule")
+ self.msg_label.set_markup("<span color='red'>" + msg + "</span>")
+ return False
+ return True
+
+ def define_filter(self, _obj):
+ self.filterdb = gramps.gen.filters.CustomFilters
+ the_filter = self.get_filter()
+ if the_filter is None:
+ self.msg_label.set_markup("<span color='red'>" + _("Supply at least one value") + "</span>")
As above.
------------------------------
In gramps/gen/filters/rules/event/__init__.py
<#1721 (comment)>
:
> @@ -71,4 +72,5 @@
HasTag,
MatchesPlaceFilter,
HasDayOfWeek,
+ HasEventBase,
How is HasEventBase and HasSourceBase related to this PR? I can't see
them referenced anywhere.
—
Reply to this email directly, view it on GitHub
<#1721 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEIYOXWT463UNOTQL4EI5OLZBZ3NNAVCNFSM6AAAAABHSDFO3WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJRGI2TCNZQG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@kkujansuu I'll finish this for you. |
They keybindings don't work on Fedora. ( |
This is an enhanced version of the Filter gramplet. The enhancements are