Skip to content

feat: implement TEXTEDIT command and TEXTEDITMODE system variable#130

Merged
HakanSeven12 merged 3 commits into
HakanSeven12:mainfrom
KarimJerbi:main
Jun 21, 2026
Merged

feat: implement TEXTEDIT command and TEXTEDITMODE system variable#130
HakanSeven12 merged 3 commits into
HakanSeven12:mainfrom
KarimJerbi:main

Conversation

@KarimJerbi

Copy link
Copy Markdown
Contributor
  • Adds TEXTEDIT command to seamlessly edit text objects in-place.
  • Matches industry-standard CAD tools' prompt behavior and "Current settings" banner logic.
  • Implements the TEXTEDITMODE system variable (0/1) for Multiple or Single editing modes.
  • command lifecycle with CmdResult::SuspendForTextEdit and post_editor_closed hooks, allowing commands to cleanly pause and resume after external UI editors close.

- Adds TEXTEDIT command to seamlessly edit annotation objects in-place.
- Matches industry-standard CAD tools' prompt behavior and "Current settings" banner logic.
- Implements the TEXTEDITMODE system variable (0/1) for Multiple vs Single editing modes.
- Enhances command lifecycle with CmdResult::SuspendForTextEdit and post_editor_closed hooks, allowing commands to cleanly pause and resume after external UI editors close.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a258f29ce

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/app/cmd_result.rs Outdated
.scene
.document
.get_entity(handle)
.map(|e| crate::app::text_inline::read_text_field(e).is_some())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve leaders before rejecting TEXTEDIT picks

Selecting a LEADER that annotates text now hits this read_text_field check and fails before begin_text_edit() can resolve Leader.annotation_handle; the double-click/DDEDIT paths explicitly allow Leader and route to begin_text_edit, so TEXTEDIT reports “selected entity is not text” for leaders with editable annotations. Let the resolver run or include Leader in the eligibility check.

Useful? React with 👍 / 👎.

Comment thread src/app/update.rs Outdated
Comment on lines +4696 to +4697
self.mtext_commit();
Task::none()
self.post_editor_closed(true)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Only count MText edits when the commit changes the drawing

When TEXTEDIT is suspended on an existing MText and the user clears the editor then presses OK, mtext_commit() returns without pushing an undo snapshot or changing the drawing for empty content, but this path still calls post_editor_closed(true). That increments edit_count, making the command's Undo option perform a document-level undo of an unrelated prior operation; only report committed=true when the MText commit actually changed the drawing.

Useful? React with 👍 / 👎.

- Custom recursive resolver ensures Leader entities trace back to their editable Text/MText annotations.

- Bypasses overly strict baseline field checks so TEXTEDIT can natively target and launch the inline editor on Leaders.
- Propagates commit status out of inline editors so commands know whether a real change occurred.

- Fixes TEXTEDIT's Undo step rolling back unrelated document actions when the user presses OK on empty/cleared existing MText.
@HakanSeven12

Copy link
Copy Markdown
Owner

Review

Solid design — the suspend/resume command lifecycle is clean and the guards are in the right places. A few findings:

🐛 Bug — invalid TEXTEDITMODE value aborts the command

In TexteditmodeCommand::on_text_input (src/modules/annotate/textedit.rs), an unrecognized value returns:

Some(CmdResult::Measurement("Requires 0 OR 1 OR MULTIPLE OR SINGLE".to_string()))

But the Measurement arm in src/app/cmd_result.rs:707 sets active_cmd = None, which ends the command. So a typo at the value prompt cancels TEXTEDITMODE instead of re-prompting. Expected: print the error and keep asking.

♻️ Duplication — 0/1/m/s/multiple/single parsed in three places

The same value parsing lives in:

  • src/app/commands.rs:4923 (direct TEXTEDITMODE <val> entry)
  • TexteditmodeCommand::on_text_input
  • src/app/settings.rs:145 (settings load)

Worth extracting a single parse_texteditmode(&str) -> Option<bool> helper.

🧹 Trailing whitespace

src/modules/annotate/textedit.rs has trailing whitespace on lines 100, 113, 121, 130, 201, 208.

✅ Looks good

  • post_editor_closed's None-guard keeps the normal MTEXT/TEXT flow safe.
  • can_edit_text's Leader resolution loop is bounded (no cycles).
  • texteditmode persists automatically via current_settings.
  • Prompt redisplay works through the NeedPoint arm, so Mode / Single-Multiple switching shows the right prompt.

No blockers. The Measurement abort is the one worth fixing before merge.

@HakanSeven12 HakanSeven12 merged commit 8c48233 into HakanSeven12:main Jun 21, 2026
@KarimJerbi

Copy link
Copy Markdown
Contributor Author

do i need to adress the duplications and mesearment arm ? sorry i took my time

@HakanSeven12

Copy link
Copy Markdown
Owner

No 🙂 I fixed it myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants