[FIX] Concordance: selection settings #249

jerneju · 2017-05-22T13:41:55Z

Issue

Fixes #217.

Description of changes

Includes

Code changes
Tests
Documentation

codecov-io · 2017-05-22T13:51:00Z

Codecov Report

Merging #249 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #249   +/-   ##
=======================================
  Coverage   85.04%   85.04%           
=======================================
  Files          34       34           
  Lines        1825     1825           
  Branches      333      333           
=======================================
  Hits         1552     1552           
  Misses        238      238           
  Partials       35       35

ajdapretnar · 2017-05-25T10:24:59Z

orangecontrib/text/widgets/owconcordance.py

    autocommit = Setting(True)
    context_width = Setting(5)
-    word = Setting("")
-    # TODO Set selection settings.
+    word = ContextSetting("b", exclude_metas=False)


Why is "b" the default? Please set it to an empty string.

Well the widget is still WIP, I have not done the cleaning yet. Anyway thanks for the comments.

ajdapretnar · 2017-05-25T10:26:30Z

orangecontrib/text/widgets/owconcordance.py

@@ -85,7 +85,7 @@ def flags(self, _):
        return Qt.ItemIsEnabled | Qt.ItemIsSelectable

    def rowCount(self, parent=None, *args, **kwargs):
-        return 0 if parent.isValid() or self.word_index is None \
+        return 0 if parent and parent.isValid() or self.word_index is None \


This still fails if I select some documents in Concordance and then remove the data. It is fixed in #232.

ajdapretnar · 2017-05-25T10:27:08Z

orangecontrib/text/widgets/owconcordance.py

@@ -223,11 +227,15 @@ def set_width(self):
                QItemSelectionModel.SelectCurrent | QItemSelectionModel.Rows)

    def set_corpus(self, data=None):
+        self.closeContext()
+


While at it, would it not make sense to remove the empty line? Or is this intentional?

ajdapretnar · 2017-05-25T10:27:24Z

orangecontrib/text/widgets/owconcordance.py

        self.corpus = data
        if data is not None and not isinstance(data, Corpus):
            self.corpus = Corpus.from_table(data.domain, data)
        self.model.set_corpus(self.corpus)
-        self.update_widget()
+        self.openContext(self.corpus)
+


Same as above.

ajdapretnar · 2017-05-25T10:31:11Z

orangecontrib/text/widgets/owconcordance.py

        else:
            self.send("Selected Documents", None)


+    def get_selection(self):


This is probably nitpicking, but perhaps we should move the two selection methods to line 229? Just trying to keep with the structure of the rest of the Text widgets. :)
Also, please remove the double empty lines above and keep double empty lines only before if name.

ajdapretnar · 2017-05-25T11:39:12Z

Doh, sorry, I didn't see the tag. 🙈 Silly me.
I was just in a hurry to merge EVERYTHING!! 🤸‍♂️ 🤣

ajdapretnar · 2017-05-26T08:28:29Z

So is this done or do we wait for similar fixes as in #241 (list issue)?
+ I think this needs a rebase?

jerneju · 2017-05-26T09:40:36Z

Rebased.

ajdapretnar · 2017-05-26T10:30:09Z

@nikicc Please check as well and merge. :)

nikicc · 2017-05-31T15:48:34Z

orangecontrib/text/widgets/owconcordance.py

@@ -235,12 +239,32 @@ def set_width(self):
            self.conc_view.selectionModel().select(sel,
                QItemSelectionModel.SelectCurrent | QItemSelectionModel.Rows)

+    def get_selection(self):
+        sel = self.conc_view.selectionModel().selection()
+        selection = []


Why not just:

selection = sorted(set(cell.row() for cell in sel.indexes()))

nikicc · 2017-05-31T15:54:37Z

orangecontrib/text/widgets/owconcordance.py

@@ -256,7 +280,7 @@ def set_word_from_input(self, topic):
    def set_word(self):
        self.model.set_word(self.word)
        self.update_widget()
-        self.commit()
+        self.set_selection()


Why this? Currently, if I am inputing different words on input the selection persists which is kind on unintuitive?

jerneju · 2017-06-01T09:52:13Z

Rows selection not finished yet. Waiting forDataHashContextHandlerto be implemented. Similar problem as in #255 .

nikicc · 2017-06-01T11:34:25Z

We should probably do the same for #256 to avoid problems with selection being stored in context settings.

ajdapretnar · 2017-07-13T13:10:02Z

@jerneju What is the state of this PR? Is it still WIP? Can we expect to merge soon?

jerneju · 2017-07-13T13:20:53Z

@ajdapretnar: It is still impossible to implement this since we cannot do it without proper context handler. See my comment above.

ajdapretnar · 2017-07-13T13:27:14Z

Ok, next step. Is anyone working on the context handler? If not, should we assign? It would be great to finish this before the workshop.

nikicc · 2017-09-01T07:44:20Z

orangecontrib/text/widgets/owconcordance.py

@@ -173,7 +173,7 @@ class Outputs:
    autocommit = Setting(True)
    context_width = Setting(5)
    word = ContextSetting("", exclude_metas=False)
-    # TODO Set selection settings (DataHashContextHandler)
+    selection = Setting([0], schema_only=True)


Why do we automatically select the first item? Shouldn't we leave the selection empty by defult?

Hm... Perhaps you are right. Selecting the first item only makes sense in Corpus Viewer, but not in Concordance. I'm sorry, this is probably my fault as I said to make it the same as CV.

Ok, let it be empty.

nikicc · 2017-09-01T09:12:15Z

orangecontrib/text/widgets/owconcordance.py

+        if self.corpus:
+            self._set_selection(selection)
+        else:
+            self._set_selection([0])


Same here, why do we set [0] as selection if there is no corpus?

nikicc · 2017-09-01T09:12:34Z

orangecontrib/text/widgets/owconcordance.py

+    def handleNewSignals(self):
+        selection = self.selection
+        if self.corpus:
+            self._set_selection(selection)


self._set_selection(self.selection)?

nikicc · 2017-09-01T09:13:09Z

orangecontrib/text/widgets/owconcordance.py

+    def _save_selection(self):
+        sel = self.conc_view.selectionModel().selection()
+        selection = sorted(set(cell.row() for cell in sel.indexes()))
+        if selection:


Why this if?

nikicc · 2017-09-01T14:31:12Z

@jerneju I added some code that clears selections when words on input change and also did some refactoring. Please check, if everything is OK.

Perhaps, @ajdapretnar can you also check this one more time?

jerneju · 2017-09-01T14:47:14Z

orangecontrib/text/widgets/owconcordance.py

        self.set_word()

    @Inputs.query_word
    def set_word_from_input(self, topic):
        self.Warning.multiple_words_on_input.clear()
+        if self.is_word_on_input:   # word changed, clear selection
+            self.selected_rows = []


So it only resets when we change the input word from one topic to another and not also when we reload the scema and topic is passed on input.

I think that without this when reloading, the selection would be reset before handleNewSignals would have the chance to set it.

jerneju · 2017-09-04T07:31:43Z

orangecontrib/text/widgets/owconcordance.py

@@ -259,17 +255,21 @@ def _set_selection(self, selection):
    def set_corpus(self, data=None):
        self.closeContext()
        self.corpus = data
-        if data is not None and not isinstance(data, Corpus):
-            self.corpus = Corpus.from_table(data.domain, data)


I think these two lines should not have been removed. Now the test fails.

nikicc · 2017-09-08T07:38:12Z

@jerneju I removed the test that doesn't apply anymore. Should be fine now.

ajdapretnar reviewed May 25, 2017

View reviewed changes

jerneju changed the title ~~[WIP][FIX] Concordance: settings~~ [FIX] Concordance: settings May 25, 2017

ajdapretnar approved these changes May 26, 2017

View reviewed changes

nikicc suggested changes May 31, 2017

View reviewed changes

jerneju changed the title ~~[FIX] Concordance: settings~~ [WIP][FIX] Concordance: selection settings Jun 1, 2017

jerneju changed the title ~~[WIP][FIX] Concordance: selection settings~~ [FIX] Concordance: selection settings Aug 31, 2017

nikicc suggested changes Sep 1, 2017

View reviewed changes

[FIX] Concordance: settings

c3d55e8

nikicc approved these changes Sep 1, 2017

View reviewed changes

jerneju commented Sep 1, 2017

View reviewed changes

jerneju commented Sep 4, 2017

View reviewed changes

Concordance: Clear selection & refactor

f464e27

jerneju merged commit 0524376 into biolab:master Sep 8, 2017

jerneju deleted the concordance-settings branch September 8, 2017 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Concordance: selection settings #249

[FIX] Concordance: selection settings #249

jerneju commented May 22, 2017

codecov-io commented May 22, 2017 •

edited

Loading

ajdapretnar May 25, 2017

jerneju May 25, 2017

ajdapretnar May 25, 2017

ajdapretnar May 25, 2017

ajdapretnar May 25, 2017

ajdapretnar May 25, 2017

ajdapretnar commented May 25, 2017

ajdapretnar commented May 26, 2017 •

edited

Loading

jerneju commented May 26, 2017

ajdapretnar commented May 26, 2017

nikicc May 31, 2017

nikicc May 31, 2017

jerneju commented Jun 1, 2017

nikicc commented Jun 1, 2017

ajdapretnar commented Jul 13, 2017

jerneju commented Jul 13, 2017

ajdapretnar commented Jul 13, 2017

nikicc Sep 1, 2017

ajdapretnar Sep 1, 2017

jerneju Sep 1, 2017

nikicc Sep 1, 2017

nikicc Sep 1, 2017

jerneju Sep 1, 2017

nikicc Sep 1, 2017

nikicc commented Sep 1, 2017

jerneju Sep 1, 2017

nikicc Sep 1, 2017 •

edited

Loading

jerneju Sep 4, 2017

nikicc commented Sep 8, 2017

[FIX] Concordance: selection settings #249

[FIX] Concordance: selection settings #249

Conversation

jerneju commented May 22, 2017

Issue

Description of changes

Includes

codecov-io commented May 22, 2017 • edited Loading

Codecov Report

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

ajdapretnar commented May 25, 2017

ajdapretnar commented May 26, 2017 • edited Loading

jerneju commented May 26, 2017

ajdapretnar commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerneju commented Jun 1, 2017

nikicc commented Jun 1, 2017

ajdapretnar commented Jul 13, 2017

jerneju commented Jul 13, 2017

ajdapretnar commented Jul 13, 2017

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

nikicc commented Sep 1, 2017

Choose a reason for hiding this comment

nikicc Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikicc commented Sep 8, 2017

codecov-io commented May 22, 2017 •

edited

Loading

ajdapretnar commented May 26, 2017 •

edited

Loading

nikicc Sep 1, 2017 •

edited

Loading