Skip to content
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

Fix text layer getting deselected after clicking out of Text tool interactive editing #2144

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Sidharth-Singh10
Copy link
Contributor

Closes #2127

  • Introduced the active_panel field in DocumentMessageHandler to track the currently active interface panel.
  • Added a match expression in set_editing fn to verify if the active panel is the Properties Panel, preventing its deselection during edits.
Screencast_20241218_015818.online-video-cutter.com.mp4

@0HyperCube 0HyperCube marked this pull request as ready for review December 17, 2024 21:25
Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

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

Works well; thanks for this contribution.

@Keavon Keavon changed the title properties panel remains active when user edits text layer Fix text layer being deselected after typing when going to edit its Properties panel, making it disappear immediately Dec 17, 2024
@Keavon Keavon requested a review from adamgerhant December 18, 2024 20:32
@Keavon
Copy link
Member

Keavon commented Dec 18, 2024

@adamgerhant could you please review this in regards to the introduced ActivePanel code, to make sure it doesn't conflict with your existing implementation of the related concept of the active panel used for showing whether the graph's subnetwork or Layers panel is active?

Copy link
Collaborator

@adamgerhant adamgerhant left a comment

Choose a reason for hiding this comment

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

The active panel is currently being stored in the portfolio message handler. There is no need to store it in the document message handler.

@0HyperCube
Copy link
Member

@adamgerhant the text tool code can't access the portfolio message handler.

@adamgerhant
Copy link
Collaborator

Why not just leave the text layer as selected when editing ends?

@Sidharth-Singh10
Copy link
Contributor Author

I haven't worked much with design tools much, but doesn't keeping text layers selected after editing deviates from standard design tool patterns where edits typically end in deselection.
Persistent selection might create risk for accidental modifications - users may unintentionally resize/move/style the text layer when they meant to work on something else.

In this implementation the layer while in editing is deselected when clicked anywhere else expect properties panel

@Keavon
Copy link
Member

Keavon commented Dec 20, 2024

@Sidharth-Singh10 I like where you're going with that thought process, although I believe for consistency with other drawing tools in Graphite, it would be best to keep it selected after clicking out. If you draw with any other tool like Rectangle or Ellipse, you'll notice if you look in the Layers panel, that layer remains selected after finishing drawing it. So it'd be best if the layer remains selected if you click anywhere in the viewport, or anywhere else in the UI, after interactively editing the text. Thanks :)

Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

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

See my previous comment for the updated approach, thanks!

@Keavon
Copy link
Member

Keavon commented Dec 20, 2024

!build

Copy link

📦 Build Complete for 2d77dcb
https://3a16e2da.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Dec 20, 2024

Can I have you add another little improvement to this PR while I'm thinking about it? If the user is editing the interactive text in the viewport, and hits escape, it should delete the entire text layer if escape was pressed when the text was empty. So if a user creates text in the wrong spot but doesn't type anything (or types then deletes it all), escape will delete the layer so it doesn't get invisibly stuck behind. In addition to the escape key, also right clicking somewhere outside the text within the viewport should have the same effect as escape.

@Keavon
Copy link
Member

Keavon commented Dec 21, 2024

!build

Copy link

📦 Build Complete for 6ecc336
https://94cb21e2.graphite.pages.dev

@Keavon
Copy link
Member

Keavon commented Dec 22, 2024

!build

@Keavon
Copy link
Member

Keavon commented Dec 22, 2024

It looks like Escape is working but right-click isn't for canceling an empty text layer for it to get deleted. Can you please look into making it work when it's empty and the user right-clicks?

(Also note that I rebased, so you'll need to pull before continuing to code on it.)

Copy link

📦 Build Complete for 60901c0
https://a8ca4611.graphite.pages.dev

@Sidharth-Singh10
Copy link
Contributor Author

Hey @Keavon, I added an entry for MouseRight with action_dispatch set to TextToolMessage::CommitText in input_mapping.rs, but it doesn't seem to have any effect.

To confirm I was editing the correct location, I commented out the MouseLeft entry with action_dispatch set to TextToolMessage::Interact. This prevented placing or editing text, which suggests my changes are indeed in the right place.

Could you nudge me in the right direction?

@Keavon
Copy link
Member

Keavon commented Dec 22, 2024

I'll ping @0HyperCube to see if he can answer your question about that.

@Keavon Keavon changed the title Fix text layer being deselected after typing when going to edit its Properties panel, making it disappear immediately Fix text layer getting selected after clicking out of Text tool interactive editing Dec 23, 2024
@Keavon Keavon changed the title Fix text layer getting selected after clicking out of Text tool interactive editing Fix text layer getting deselected after clicking out of Text tool interactive editing Dec 23, 2024
@0HyperCube
Copy link
Member

@Sidharth-Singh10

The actions that are advertised to the input mapper system don't include the commit text when the text is not being edited:

fn actions(&self) -> ActionList {
	match self.fsm_state {
		TextToolFsmState::Ready => actions!(TextToolMessageDiscriminant;
			Interact,
		),
		TextToolFsmState::Editing => actions!(TextToolMessageDiscriminant;
			Interact,
			Abort,
			CommitText,
		),
	}
}

Any time any mouse button is pressed that is not inside the text input, input.ts:175 sends editor.handle.onChangeText which sets editing to false.

@Sidharth-Singh10
Copy link
Contributor Author

Thanks for pointing this out!

@Keavon
Copy link
Member

Keavon commented Dec 23, 2024

!build

Copy link

📦 Build Complete for a29a3a8
https://dbab0636.graphite.pages.dev

@Keavon Keavon requested a review from Copilot December 23, 2024 19:12

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

editor/src/messages/tool/tool_messages/text_tool.rs:257

  • The check for has_remove_textbox should be done before adding DisplayRemoveEditableTextbox to the responses to avoid redundancy.
let has_remove_textbox = responses.iter().any(|msg| matches!(msg, Message::Frontend(FrontendMessage::DisplayRemoveEditableTextbox)));

editor/src/messages/tool/tool_messages/text_tool.rs:467

  • The set_editing call should be made after deleting the empty text layer to prevent potential UI issues.
tool_data.set_editing(false, font_cache, responses);
@Keavon
Copy link
Member

Keavon commented Dec 23, 2024

@Sidharth-Singh10 your solution to emulate an escape keypress from the frontend TS code is a hack that works but isn't very clean. Can you please take into account what Hypercube wrote and implement a solution in the backend Rust code? In other words, you need to have something run before the tool state leaves Editing and returns to Ready to check if that TS code he mentioned is causing it to return to Ready.

@Keavon
Copy link
Member

Keavon commented Dec 24, 2024

!build

Copy link

📦 Build Complete for 2686957
https://bcbcd48f.graphite.pages.dev

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.

Keep selection when clicking in properties after creating text
4 participants