Fix #14 #142

rassouly · 2018-05-17T12:53:18Z

Enforce unique names when creating a task
Enforce unique names when renaming a task
Update/add tests

Known issues:

Right click->Add after in the task tree bypasses uniqueness checks.
Copy-pasting doesn't work: the pasted task has the same name as the initial task. This seems hard to fix in a nice way because the behavior of PasteAction (in qt_tree_menu.enaml) is not overridable. EDIT: Found a hacky solution. EDIT2: Found a nicer solution.

Comment are welcome.

…ew task.

MatthieuDartiailh · 2018-05-17T19:21:19Z

exopy/utils/widgets/tree_nodes.py

@@ -177,7 +177,10 @@ def exit_rename(self, obj, label):

        """
        label_name = self.label
-        if label_name[:1] != '=':


This is not from my point of view the way to do it. It is better to override exit_rename in the task tree nodes (declared in the measure_editor). Those function exist to allow such customizations.

MatthieuDartiailh · 2018-05-17T20:22:13Z

exopy/measurement/workspace/measurement_edition.enaml

+    exit_rename => (obj, label):
+        forbidden_names = obj.root.get_used_names()
+        if label not in forbidden_names:
+            obj.name = label


We could consider showing a popup explaining why renaming failed... But do not bother with this, opening an issue will be enough.

…tation.

codecov · 2018-05-25T09:56:38Z

Codecov Report

Merging #142 into master will decrease coverage by 0.01%.
The diff coverage is 93.44%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
- Coverage   98.38%   98.37%   -0.02%     
==========================================
  Files         157      157              
  Lines       12292    12335      +43     
==========================================
+ Hits        12093    12134      +41     
- Misses        199      201       +2

MatthieuDartiailh

I realized (a bit late) that this deviates a bit from my initial design. I would prefer to work with the direct parent rather than the root and to be future-proof it would be interesting to also pass the future index of the task in its parent. Sorry for the noticing so late.

MatthieuDartiailh · 2018-05-25T11:51:48Z

exopy/utils/widgets/qt_tree_widget.py

@@ -783,8 +783,7 @@ def _on_nid_changed(self, nid, col):
        if new_label != old_label:
            if new_label != '':
                node.exit_rename(obj, new_label)
-            else:
-                self._set_label(nid, old_label)
+            self._set_label(nid, node.get_label(obj))


Why this change ?

When the new name is invalid (because it's not unique), exit_rename doesn't set obj.name to new_label. The issue is, without this patch, the name displayed in the tree would be stuck on the new_label, even though it's invalid and doesn't reflect the actual name.
The systematic call to _set_label syncs up the two.

MatthieuDartiailh · 2018-05-25T11:56:23Z

exopy/tasks/tasks/base_views.enaml

@@ -88,13 +88,11 @@ enamldef RootTaskView(BaseTaskView): main:
        BaseTaskView.refresh(self)
        editor.refresh()

-    # TODO pass the future parent task so that config can do name inspections
-    # and avoid duplicate names.
    func create_new_task():


Looking at the full diff, I realized I would prefer to follow more closely what I was suggesting in this TODO and pass the future parent rather than the node in case we decide to ever use a different validation or need it for specific config.

I don't want to pass the future parent because I want the names to be unique in the entire tree. This makes moving nodes in the tree easier.

You can easily get the root from the parent and having the parent offer more possibilities.

Ok, makes sense, I am reworking the PR.

Ok let me know if you need help with anything.

MatthieuDartiailh · 2018-05-25T11:58:38Z

exopy/tasks/configs/base_configs.py

@@ -47,15 +50,15 @@ class BaseTaskConfig(Atom):

    def __init__(self, **kwargs):
        super(BaseTaskConfig, self).__init__(**kwargs)
-        # Force check to ensure that the possible default value of task_name
-        # is tested.
-        self.check_parameters()


From my point of view this could be problematic in subclasses if we need to validate something else than the name. It is not the job of the default value builder to validate the name.

I needed to remove the check_parameters to prevent an infinite loop. I can't exactly remember why it happened but moving the check from __init__ to _default_task_name solved the issue.
I can look at this more closely if you want.

It seems that reworking the PR fixed the issue so I am putting the check_parameters back.

MatthieuDartiailh · 2018-05-25T11:58:53Z

exopy/tasks/configs/base_configs.py

@@ -78,7 +81,10 @@ def _post_setattr_task_name(self, old, new):
    def _default_task_name(self):
        names = self.manager.auto_task_names
        if names:
-            return random.choice(names)
+            name = random.choice(names)


See my above comment.

MatthieuDartiailh · 2018-05-25T12:01:00Z

exopy/tasks/configs/base_configs.py

+        names = []
+        if self.root:
+            names = self.root.get_used_names()
+        self.ready = self.task_name != "" and self.task_name not in names


To help the user, it would be nice if the UI could provide some feedback (such as a red background for the name). This would require to be able to only validate the name (not hard since you can store the name on the config to make the check cheap).

Alternatively you can add a valid_name Bool member.

MatthieuDartiailh · 2018-05-25T12:01:53Z

exopy/tasks/widgets/building.enaml

@@ -38,6 +38,8 @@ enamldef BuilderView(Dialog): dial:
    #: Reference to the task manager.
    alias manager : selector.manager

+    attr root


This would be nicer with a comment.

- Replace the root attribute in the task configuration window by a future_parent attribute which contains the future parent of the task being created. - Add visual feedback when the name is not valid (this information is also stored in the config object) - Readd the check_parameters call in the BaseConfig constructor.

rassouly · 2018-05-26T12:41:02Z

The tests fail on Python 3.5. It seems that future_parent is set before manager in
config = PyTaskConfig(manager=plugin, task_class=plugin.get_task('exopy.ComplexTask'), future_parent=root)
How can I get atom to set manager before future_parent? Is it undefined behavior?

MatthieuDartiailh · 2018-05-26T16:14:06Z

No you can't since keywords arguments are not ordered. I will look at this either tomorrow or Monday.

rassouly · 2018-05-26T16:51:07Z

Please note that this issue doesn't apply to the actual code of exopy but only to the tests. Indeed, in the code, the future_parent is set after the config object has been created (see building.enaml line 58).

MatthieuDartiailh

I made a small that should allow tests to always pass. I left two minor comments but otherwise this is good to go I believe.

MatthieuDartiailh · 2018-05-27T21:04:49Z

exopy/tasks/configs/base_config_views.enaml

@@ -34,6 +35,7 @@ enamldef BaseConfigView(Container):
    Field: t_f:
        text := config.task_name
        submit_triggers = ['lost_focus', 'return_pressed', 'auto_sync']
+        background << parse_color("#FFFFFF") if config.name_valid else parse_color("#FF000030")


I believe that 'white' and 'red' would have been fine too (and easier to read).

I used some transparency to make the red softer and I used the HTML notation for the white for constency.

MatthieuDartiailh · 2018-05-27T21:12:35Z

exopy/tasks/configs/base_configs.py

    def __init__(self, **kwargs):
        super(BaseTaskConfig, self).__init__(**kwargs)
-        # Force check to ensure that the possible default value of task_name
-        # is tested.


Could you put those two lines back ?

MatthieuDartiailh · 2018-05-27T22:01:30Z

I added two blank space by editing on Github (loop_cofig) Can you remove them along the other minor edits ?

MatthieuDartiailh · 2018-05-28T08:07:04Z

Thanks @rassouly I will try to review one last time before merging but I believe that this is good to go if Travis comes back green.

MatthieuDartiailh · 2018-05-31T20:09:47Z

I read through again and it looks good, but I will try to test directly before merging.

MatthieuDartiailh · 2018-07-01T14:31:53Z

Thanks @rassouly

Enforce a unique name for each task in each task tree when adding a n…

e0603b7

…ew task.

rassouly force-pushed the fix-14 branch from 7af4d9d to e0603b7 Compare May 17, 2018 13:02

rassouly added 2 commits May 17, 2018 15:40

Ignore TaskInterface objects in the task tree

d7b2892

Renaming: Enforce a unique name for each task in the task tree

6d0466d

MatthieuDartiailh reviewed May 17, 2018

View reviewed changes

Renaming: Override exit_rename instead of modifying it.

658e2fa

MatthieuDartiailh reviewed May 17, 2018

View reviewed changes

Fix parameter naming inconsistency in create_task. Update the documen…

662ad77

…tation.

rassouly force-pushed the fix-14 branch from 472414f to 662ad77 Compare May 17, 2018 20:26

Renaming: Check name uniqueness when adding a task from the tree widget.

22e488f

rassouly mentioned this pull request May 17, 2018

Add a popup when renaming fails #144

Open

measurement_edition: Prefix the name of tasks that are copy-pasted.

35ac6c9

rassouly force-pushed the fix-14 branch from b5bd903 to 35ac6c9 Compare May 18, 2018 13:29

Add new tests and update old ones

376c0cc

rassouly changed the title ~~[WIP] Fix #14~~ Fix #14 May 25, 2018

MatthieuDartiailh requested changes May 25, 2018

View reviewed changes

rassouly added 2 commits May 25, 2018 19:57

Removed unused import

292a959

tasks: run checks only when manager is known

d9d2b8e

MatthieuDartiailh requested changes May 27, 2018

View reviewed changes

Update loop_config.py

99559fc

Add comment and delete whitespace

a1b6037

rassouly mentioned this pull request Jun 20, 2018

Fix #143 #152

Merged

MatthieuDartiailh approved these changes Jul 1, 2018

View reviewed changes

MatthieuDartiailh merged commit 170ad2d into Exopy:master Jul 1, 2018

rassouly deleted the fix-14 branch July 2, 2018 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #14 #142

Fix #14 #142

rassouly commented May 17, 2018 •

edited

Loading

MatthieuDartiailh May 17, 2018

MatthieuDartiailh May 17, 2018

codecov bot commented May 25, 2018 •

edited

Loading

MatthieuDartiailh left a comment

MatthieuDartiailh May 25, 2018

rassouly May 25, 2018 •

edited

Loading

MatthieuDartiailh May 25, 2018

rassouly May 25, 2018 •

edited

Loading

MatthieuDartiailh May 25, 2018

rassouly May 25, 2018

MatthieuDartiailh May 25, 2018

MatthieuDartiailh May 25, 2018

rassouly May 25, 2018 •

edited

Loading

rassouly May 25, 2018 •

edited

Loading

MatthieuDartiailh May 25, 2018

MatthieuDartiailh May 25, 2018

MatthieuDartiailh May 25, 2018

MatthieuDartiailh May 25, 2018

rassouly commented May 26, 2018 •

edited

Loading

MatthieuDartiailh commented May 26, 2018

rassouly commented May 26, 2018

MatthieuDartiailh left a comment

MatthieuDartiailh May 27, 2018

rassouly May 28, 2018

MatthieuDartiailh May 28, 2018

MatthieuDartiailh May 27, 2018

MatthieuDartiailh commented May 27, 2018

MatthieuDartiailh commented May 28, 2018

MatthieuDartiailh commented May 31, 2018

MatthieuDartiailh commented Jul 1, 2018

Fix #14 #142

Fix #14 #142

Conversation

rassouly commented May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 25, 2018 • edited Loading

Codecov Report

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rassouly May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rassouly May 25, 2018 • edited Loading

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

rassouly May 25, 2018 • edited Loading

Choose a reason for hiding this comment

rassouly May 25, 2018 • edited Loading

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

rassouly commented May 26, 2018 • edited Loading

MatthieuDartiailh commented May 26, 2018

rassouly commented May 26, 2018

MatthieuDartiailh 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

MatthieuDartiailh commented May 27, 2018

MatthieuDartiailh commented May 28, 2018

MatthieuDartiailh commented May 31, 2018

MatthieuDartiailh commented Jul 1, 2018

rassouly commented May 17, 2018 •

edited

Loading

codecov bot commented May 25, 2018 •

edited

Loading

rassouly May 25, 2018 •

edited

Loading

rassouly May 25, 2018 •

edited

Loading

rassouly May 25, 2018 •

edited

Loading

rassouly May 25, 2018 •

edited

Loading

rassouly commented May 26, 2018 •

edited

Loading