-
Notifications
You must be signed in to change notification settings - Fork 20
feat: add renaming frames functionality, fix: crash when trying to delete a group #51
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
base: noetic-devel
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -188,7 +188,61 @@ def undo(self): | |||||||||||||||
| self.element.hidden = True | ||||||||||||||||
| self.editor.add_undo_level(1, [self.element]) | ||||||||||||||||
|
|
||||||||||||||||
| class Command_RenameElement(QUndoCommand): | ||||||||||||||||
| '''Copys a source frame's transformation and sets a new parent | ||||||||||||||||
| ''' | ||||||||||||||||
|
|
||||||||||||||||
| def __init__(self, editor, element, new_name): | ||||||||||||||||
| QUndoCommand.__init__(self, "Rename") | ||||||||||||||||
| self.editor = editor | ||||||||||||||||
| self.element = element | ||||||||||||||||
|
|
||||||||||||||||
| if editor.active_frame is element: | ||||||||||||||||
| self.was_active = True | ||||||||||||||||
| else: | ||||||||||||||||
| self.was_active = False | ||||||||||||||||
|
|
||||||||||||||||
| self.new_name = new_name | ||||||||||||||||
| self.old_name = element.name | ||||||||||||||||
|
|
||||||||||||||||
| self.element.name = self.new_name | ||||||||||||||||
|
|
||||||||||||||||
| def redo(self): | ||||||||||||||||
| if self.was_active: | ||||||||||||||||
| self.editor.active_frame = self.element | ||||||||||||||||
| self.editor.add_undo_level(2) | ||||||||||||||||
|
|
||||||||||||||||
| # Remove old frame | ||||||||||||||||
| if self.editor.frames.get(self.old_name): | ||||||||||||||||
| del self.editor.frames[self.old_name] | ||||||||||||||||
|
||||||||||||||||
| if self.editor.frames.get(self.old_name): | |
| del self.editor.frames[self.old_name] | |
| self.editor.frames.pop(self.old_name, None): |
or
| if self.editor.frames.get(self.old_name): | |
| del self.editor.frames[self.old_name] | |
| if self.old_name in self.editor.frames: | |
| del self.editor.frames[self.old_name] |
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.
Done. Changed it to self.editor.frames.pop(self.old_name, None)
Outdated
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.
see above
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.
Done. Implemented suggestion
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ def __init__(self, frame_editor): | |
| rospy.Service("~set_frame", SetFrame, self.callback_set_frame) | ||
| rospy.Service("~set_parent", SetParentFrame, self.callback_set_parent_frame) | ||
| rospy.Service("~copy_frame", CopyFrame, self.callback_copy_frame) | ||
| rospy.Service("~rename_frame", RenameFrame, self.callback_rename_frame) | ||
|
|
||
| rospy.Service("~load_yaml", LoadYaml, self.callback_load_yaml) | ||
| rospy.Service("~save_yaml", SaveYaml, self.callback_save_yaml) | ||
|
|
@@ -276,5 +277,36 @@ def callback_copy_frame(self, request): | |
| response.error_code = 9 | ||
|
|
||
| return response | ||
|
|
||
| def callback_rename_frame(self, request): | ||
| rospy.loginfo("> Request to rename frame '{}' with new name '{}'".format(request.source_name, request.new_name)) | ||
|
|
||
| response = RenameFrameResponse() | ||
| response.error_code = 0 | ||
|
|
||
| if request.new_name == "": | ||
| rospy.logerr(" Error: No name given") | ||
| response.error_code = 1 | ||
|
|
||
| elif request.source_name == "": | ||
| rospy.logerr(" Error: No source name given") | ||
| response.error_code = 3 | ||
|
|
||
| elif request.source_name not in self.editor.frames: | ||
| rospy.logerr(f" Error: Frame not found: {request.source_name}") | ||
| response.error_code = 2 | ||
|
|
||
| elif request.new_name in self.editor.frames: | ||
| rospy.logerr(f" Error: New frame name already exists: {request.new_name}") | ||
| response.error_code = 4 | ||
|
|
||
| else: | ||
| try: | ||
| self.editor.command(Command_RenameElement(self.editor, self.editor.frames[request.source_name], request.new_name)) | ||
| except Exception as e: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you specify the exception a bit more? I think here it should be key and attribute errors There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I added the |
||
| rospy.logerr("Error: unhandled exception {}".format(e)) | ||
| response.error_code = 9 | ||
|
|
||
| return response | ||
|
|
||
| # eof | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,7 @@ def create_main_widget(self): | |
| ## | ||
| widget.btn_add.clicked.connect(self.btn_add_clicked) | ||
| widget.btn_delete.clicked.connect(self.btn_delete_clicked) | ||
| widget.btn_rename.clicked.connect(self.btn_rename_clicked) | ||
| widget.btn_duplicate.clicked.connect(self.btn_duplicate_clicked) | ||
| widget.list_frames.currentItemChanged.connect(self.selected_frame_changed) | ||
| widget.list_tf.currentItemChanged.connect(self.update_measurement) | ||
|
|
@@ -151,6 +152,7 @@ def create_main_widget(self): | |
| widget.txt_c.editingFinished.connect(self.c_valueChanged) | ||
|
|
||
| widget.txt_group.editingFinished.connect(self.group_valueChanged) | ||
| widget.txt_name.editingFinished.connect(self.frameName_valueChanged) | ||
|
|
||
| widget.btn_rad.toggled.connect(self.update_fields) | ||
|
|
||
|
|
@@ -559,6 +561,24 @@ def btn_duplicate_clicked(self, checked): | |
|
|
||
| self.editor.command(Command_CopyElement(self.editor, name, source_name, parent_name)) | ||
| self.signal_update_tf.emit(False, False) | ||
|
|
||
| @Slot(bool) | ||
| def btn_rename_clicked(self, checked): | ||
| item = self.widget.list_frames.currentItem() | ||
| if not item: | ||
| return | ||
|
|
||
| # Check if the item is a frame. Early returns if selected item is not a frame (e.g. a group) | ||
| source_name = item.text(0) | ||
| if self.editor.frames.get(source_name) is None: | ||
| return | ||
|
|
||
| new_name = self.get_valid_frame_name("Rename Frame", default_name=source_name) | ||
| if not new_name: | ||
| return | ||
|
|
||
| self.editor.command(Command_RenameElement(self.editor, self.editor.frames[source_name], new_name)) | ||
| self.signal_update_tf.emit(True, True) | ||
|
|
||
| def get_sleep_time(self): | ||
| return max(5.0 / self.editor.hz, 0.1) | ||
|
|
@@ -585,7 +605,13 @@ def btn_delete_clicked(self, checked): | |
| item = self.widget.list_frames.currentItem() | ||
| if not item: | ||
| return | ||
| self.editor.command(Command_RemoveElement(self.editor, self.editor.frames[item.text(0)])) | ||
|
|
||
| # Check if the item is a frame. Early returns if selected item is not a frame (e.g. a group) | ||
| source_name = item.text(0) | ||
| if self.editor.frames.get(source_name) is None: | ||
| return | ||
|
|
||
| self.editor.command(Command_RemoveElement(self.editor, self.editor.frames[source_name])) | ||
| self.signal_update_tf.emit(True, True) | ||
|
|
||
| ## PARENTING ## | ||
|
|
@@ -714,7 +740,24 @@ def group_valueChanged(self): | |
| if self.editor.active_frame.group != value: | ||
| self.editor.command(Command_SetGroup(self.editor, self.editor.active_frame, value)) | ||
|
|
||
| @Slot() | ||
| def frameName_valueChanged(self): | ||
| item = self.widget.list_frames.currentItem() | ||
| if not item: | ||
| return | ||
| source_name = item.text(0) | ||
|
|
||
| new_name = self.widget.txt_name.text() | ||
| existing_tf_frames = set(self.editor.all_frame_ids()) | ||
| existing_editor_frames = set(self.editor.all_editor_frame_ids()) | ||
|
Comment on lines
+751
to
+752
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think it is required to make them into a set if you just check if an element is in the list There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still Open. |
||
|
|
||
| # allow recreating if frame was published by frameditor node originally | ||
| if new_name in existing_editor_frames or (new_name in existing_tf_frames and not Frame.was_published_by_frameeditor(new_name)): | ||
|
||
| self.widget.txt_name.setText(source_name) | ||
| return None | ||
|
|
||
| self.editor.command(Command_RenameElement(self.editor, self.editor.frames[source_name], new_name)) | ||
| self.signal_update_tf.emit(True, True) | ||
|
|
||
| ## FRAME STYLE ## | ||
| ## | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| string source_name | ||
| string new_name | ||
| --- | ||
| int32 error_code |
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.
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.
Done. Implemented the suggestion