SelectRows: Add calendar popup for TimeVariables #4800

aturanjanin · 2020-05-21T14:18:40Z

Issue

Fixes #4676.

Description of changes

Calendar popup added when setting dates for TimeVariables filtering.

Includes

Code changes
Tests
Documentation

janezd · 2020-05-21T17:30:44Z

Please don't merge before #4798.

There will be conflicts and #4798 is more complex than #4800, so I'd appreciate if rebasing is left for #4800.

janezd · 2020-05-21T17:36:38Z

Orange/widgets/data/owselectrows.py

@@ -68,7 +67,7 @@ def decode_setting(self, setting, value, domain=None):
                op, values = condition[-2:]

                var = attr in domain and domain[attr]
-                if var and var.is_continuous and not isinstance(var, TimeVariable):
+                if var and var.is_continuous:


Be careful when solving the conflict with #4798: there's no var but just attr, and it can be a str or Variable, so you probably want something like

if isinstance(attr, ContinuousVariable) \ or isinstance(attr, str) and OWSelectRows.AllTypes.get(attr) == CONTINUOUS

ajdapretnar · 2020-05-22T08:55:51Z

@aturanjanin #4798 is now merged, so please rebase this and I will check. Thanks! :)

codecov · 2020-05-22T10:17:59Z

Codecov Report

Merging #4800 into master will decrease coverage by 0.06%.
The diff coverage is 68.62%.

@@            Coverage Diff             @@
##           master    #4800      +/-   ##
==========================================
- Coverage   84.20%   84.14%   -0.07%     
==========================================
  Files         282      277       -5     
  Lines       57342    56633     -709     
==========================================
- Hits        48285    47653     -632     
+ Misses       9057     8980      -77

aturanjanin · 2020-05-22T10:18:00Z

@ajdapretnar I've just rebased, could you check if everything looks okay? Thanks!

ajdapretnar · 2020-05-25T07:47:30Z

I have check this and I have a couple of comments.

Data range in calendar should be the data range of actual data. Now, I can select dates that don't exist in the data set.
I can select from 2010-01-30 to 2010-01-01. I shouldn't be able to select crossing dates. An option for selecting 'between' and 'outside' could be also the quite standard data range of online platforms. So the first click is the first date, the second click marks the end of the range. If this is too difficult to implement, never mind.
If my time variable is in hours instead of days, I cannot select the hour range.
Between and Outside options have strange GUI. They get too narrow. If I try to enlarge the window, the middle condition is resized, not the calendar. Middle condition should be of fixed size and condition resizable.
This change desperately needs tests. :)

ajdapretnar · 2020-06-02T09:27:08Z

@janezd We're in a pickle here and need your expert opinion/suggestion.
If time variable has only hours, how do we handle it in the view?
If TV has both dates and hours and I want to select hours, too, how to handle this?

ATM, the widget adds dummy date or time to variables, e.g. 1970-01-01 if data has only hours and 00:00:00 if it has only dates.

janezd · 2020-06-02T15:03:49Z

... need your expert opinion/suggestion

I wonder in what sense am I an expert here? I don't use time variables and I never heard about QDateEdit before. :)

On the first glance, you would need QDateTimeEdit, not QDateEdit. There you would set the desired display format that would show only date or only hours and minutes or both. It is also possible to control the pop-up that is shown by overriding QDateTimeEdit.calendarWidget. The pop-up can probably be replaced with one that shows only hours, minutes and seconds when desired. It may also be possible to extend CalendarWidget with line edits for hours, minutes and seconds, when all are needed.

But I'm just reading the docs and guessing -- which about rounds up my expertise. :)

janezd · 2020-06-12T11:11:22Z

@aturanjanin, I added a commit that adds time to the popup. Now I leave to you to actually connect it, that is, set defaults and connect the signals so that date changes when the user changes the numbers.

janezd

A general summary: it is better if child widgets know and assume less about parent. And, in particular, it is up to recepient of the signal to connect to the receiver's signal; making a connection is not a task for sender. Also, as I understand, you sometimes call connect instead of emit.

janezd · 2020-06-18T15:36:52Z

Orange/widgets/data/owselectrows.py

+        self.set_format()
+        self.setSizePolicy(
+            QSizePolicy(QSizePolicy.Expanding, QSizePolicy.Expanding))
+        self.dateTimeChanged.connect(parent._invalidate_dates)


Not like this. The idea of signals is that the object that needs to be informed about something "subscribes" to the signal. Remove this line and instead connect to the signal when you construct the widget, e.g.

w = DateTimeWidget(self, var_idx, datetime_format) w.dateTimeChanged.connect(self._invalidate_dates)

As it is now, this widget assumes that the parent has a method _invalidate_dates (and, besides, it starts with underscore, so it's kind of private). With the signals, method naming is up to parent. And nobody touches its private methods.

janezd · 2020-06-18T15:38:44Z

Orange/widgets/data/owselectrows.py

+            self.setDate(datetime if datetime else self.min_datetime)
+        elif not self.have_date and self.have_time:
+            self.setTime(datetime if datetime else self.min_datetime)
+        self.dateTimeChanged.connect(self.parent.conditions_changed)


Did you mean to connect the signal or emit it? I suppose you wanted to do this:

self.dateTimeChanged.emit()

To call (or connect) the proper method is up to the parent widget.

janezd · 2020-06-18T15:41:11Z

Orange/widgets/data/owselectrows.py

+        self.timeedit.dateTimeChanged.connect(self.set_time)
+
+    def set_time(self):
+        self.parent.set_datetime(self.parent.date(), self.timeedit.time())


Don't call parent's methods. It's better to emit signals and let the parent connect to them. This way, the widget doesn't have to make assumptions about its parent and the parent's methods.

janezd · 2020-06-18T15:42:24Z

Orange/widgets/data/owselectrows.py

+        sublay.addWidget(self.timeedit)
+        sublay.addStretch(1)
+        self.layout().addLayout(sublay)
+        self.parent = self.parent()


Don't store the parent. First, it is accessible as self.parent(). Second, you don't need it if you use signals instead (see below).

janezd · 2020-06-18T15:43:34Z

Orange/widgets/data/owselectrows.py

            elif vtype == 3:  # string:
                box.controls = [add_textual(lc[0])]
                if oper in [6, 7]:
                    gui.widgetLabel(box, " and ")
                    box.controls.append(add_textual(lc[1]))
+            elif vtype == 4:  # time:
+                datetime_format = (var.have_date, var.have_time)
+                w = DateTimeWidget(self, var_idx, datetime_format)


Here, connect w's signals to _invalidate_dates. (See the bottom of the file for a longer comment.)

janezd · 2020-06-18T15:43:40Z

Orange/widgets/data/owselectrows.py

+                self._box = box
+                if oper > 5:
+                    gui.widgetLabel(box, " and ")
+                    w_ = DateTimeWidget(self, var_idx, datetime_format)


janezd · 2020-06-18T15:44:54Z

Orange/widgets/data/owselectrows.py

+            w_ = self._box.controls[1]
+            if w.dateTime() > w_.dateTime():
+                w_.setDateTime(w.dateTime())
+                w_.dateTimeChanged.connect(self.conditions_changed)


Huh, this reconnects the signal at every change?! You wanted to connect the method or to just call it?

janezd · 2020-06-18T15:46:55Z

Orange/widgets/data/owselectrows.py

+    def __init__(self, parent, col_idx, datetime_format):
+        QDateTimeEdit.__init__(self, parent)
+
+        self.parent = parent


If you use signals, you won't need to store self.parent.

janezd · 2020-06-18T15:47:41Z

Orange/widgets/data/owselectrows.py

+        self.parent = parent
+        self.format = datetime_format
+        self.have_date, self.have_time = datetime_format[0], datetime_format[1]
+        self.column = parent.data[:, col_idx]


I'd be happier if column was passed as an argument, so the widget wouldn't need to assume how the parent stores data.

janezd · 2020-06-26T10:28:19Z

I apologize for doing it instead of showing you what to do.

I just wanted to fix some issues reported by lint. One of them was that attribute _box is defined outside __init__. Then I realized that _box is problematic as such: each row with conditions would need to have its own _box. It's not a property of a widget but of a single line. I couldn't construct a case where this would cause problems, though. I don't understand why the widget still worked. :)

However, instead of defining _box in __init__, I moved invalidate_datetime and datetime_changed into set_new_values. This makes set_new_values even longer, but allows invalide_datetime and datetime_changed to access w and w_ as local variable (within closure). So we no longer need to define _box, and it also has a separate w and w_ for each row of conditions.

I recommend that you check this code, you may find the trick useful.

…lint

lanzagar · 2020-07-02T09:21:20Z

Orange now crashes (after this PR was merged) when using Select Rows and selecting a Time variable from metas (but not if the time var is a feature or class...).

To reproduce:
Datasets (select Online Retail) -> Select Rows (select variable InvoiceDate)

janezd · 2020-07-02T09:32:14Z

It would be better if you wrote this as an issue, we'll ovelook it here. (I'll do it.)

janezd reviewed May 21, 2020

View reviewed changes

aturanjanin force-pushed the selectrows branch from 949cd18 to b06ea94 Compare May 22, 2020 10:02

aturanjanin changed the title ~~[ENH] SelectRows: Add calendar popup for TimeVariables~~ [WIP] SelectRows: Add calendar popup for TimeVariables May 25, 2020

aturanjanin force-pushed the selectrows branch from b06ea94 to 5fc2f25 Compare June 8, 2020 11:39

janezd self-assigned this Jun 12, 2020

janezd removed their assignment Jun 12, 2020

aturanjanin force-pushed the selectrows branch from 9a37e9a to 3a52b38 Compare June 17, 2020 10:43

aturanjanin changed the title ~~[WIP] SelectRows: Add calendar popup for TimeVariables~~ SelectRows: Add calendar popup for TimeVariables Jun 17, 2020

janezd reviewed Jun 18, 2020

View reviewed changes

aturanjanin changed the title ~~SelectRows: Add calendar popup for TimeVariables~~ [WIP] SelectRows: Add calendar popup for TimeVariables Jun 22, 2020

aturanjanin force-pushed the selectrows branch from 3a52b38 to 63340e3 Compare June 25, 2020 11:48

aturanjanin changed the title ~~[WIP] SelectRows: Add calendar popup for TimeVariables~~ SelectRows: Add calendar popup for TimeVariables Jun 25, 2020

janezd self-assigned this Jun 26, 2020

aturanjanin and others added 3 commits June 26, 2020 12:29

OWSelectRows: Add calendar popup for TimeVariables

f1158bd

Select Rows: Add time to calendar widget popup

23a4f89

OWSelectRows: Connect time in calendar; Add tests

8a5671c

janezd force-pushed the selectrows branch 2 times, most recently from b70c44b to 5cb0af0 Compare June 26, 2020 10:44

Select Rows: make invalidate_datetime uses variables from closure; py…

5cb0af0

…lint

janezd merged commit 4e6360e into biolab:master Jun 26, 2020

janezd mentioned this pull request Jul 2, 2020

Select Rows crashes for time variable in metas #4874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SelectRows: Add calendar popup for TimeVariables #4800

SelectRows: Add calendar popup for TimeVariables #4800

aturanjanin commented May 21, 2020 •

edited

Loading

janezd commented May 21, 2020

janezd May 21, 2020

ajdapretnar commented May 22, 2020

codecov bot commented May 22, 2020 •

edited

Loading

aturanjanin commented May 22, 2020

ajdapretnar commented May 25, 2020

ajdapretnar commented Jun 2, 2020

janezd commented Jun 2, 2020

janezd commented Jun 12, 2020

janezd left a comment

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd Jun 18, 2020

janezd commented Jun 26, 2020

lanzagar commented Jul 2, 2020

janezd commented Jul 2, 2020

SelectRows: Add calendar popup for TimeVariables #4800

SelectRows: Add calendar popup for TimeVariables #4800

Conversation

aturanjanin commented May 21, 2020 • edited Loading

Issue

Description of changes

Includes

janezd commented May 21, 2020

Choose a reason for hiding this comment

ajdapretnar commented May 22, 2020

codecov bot commented May 22, 2020 • edited Loading

Codecov Report

aturanjanin commented May 22, 2020

ajdapretnar commented May 25, 2020

ajdapretnar commented Jun 2, 2020

janezd commented Jun 2, 2020

janezd commented Jun 12, 2020

janezd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janezd commented Jun 26, 2020

lanzagar commented Jul 2, 2020

janezd commented Jul 2, 2020

aturanjanin commented May 21, 2020 •

edited

Loading

codecov bot commented May 22, 2020 •

edited

Loading