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
Snaps drop data to cursor when within +/- 3 columns and same line #31415
Conversation
Instead of editor setting, wouldn't it be better if it was done with a modifier key, like Shift or Control? |
Yeah, possibly. |
Hm, more settings nobody will ever know about... I think most users don't even know that they can drag and drop files to the script editor to get their path, so customizing what should happen then is a step further... IMO we should just pick which behavior is the most expected one and keep only that. |
If were not gonna have an option, reverting it back to cursor and not mouse position in my opinion seems to make better sense because you really need that precision and if you miss it by even 1 character it's a big deal. |
No having this as an option is a bad idea IMHO. This would be another setting for something that is not relevant that much. Having the drop at mouse position is the most expected behavior. If the dropping on a specific position is difficult, we should find a way to dynamically snap the dropping position to the positions that make more sense. (it might include the cursor's position) |
@groud We could use the cursor position only if the mouse position is close enough from the cursor (say, |
Yeah, that would be a good idea. ^^ |
That sounds good, as long as there's visual feedback to the user that the position is being snapped, so that it doesn't appear random. |
@sparkart Are you able to update the PR to implement #31415 (comment) ? |
I have been busy with the start of the new semester. Right now I've updated the PR to address the originating issue of being difficult to drag with precision (e.g. between parenthesis) by checking if the mouse is on the same line as the cursor and is within +/- 3 columns. Without visual feedback snapping to the cursor with a larger difference between the cursor and mouse of something like 50 pixels would indeed seem rather random. If anyone is interested in adding that feel free, if not... I'll definitely update the PR to address #31415 (comment) when I have the time. |
Adds a setting for the text editor that allows you to drop data at the mouse position or the cursor position.
1fff0ba
to
5f177a0
Compare
@@ -1473,6 +1473,8 @@ void ScriptTextEditor::drop_data_fw(const Point2 &p_point, const Variant &p_data | |||
int row, col; | |||
te->_get_mouse_pos(p_point, row, col); | |||
|
|||
bool dropAtCursor = (col <= te->cursor_get_column() + 3 && col >= te->cursor_get_column() - 3 && row == te->cursor_get_line()) ? true : false; |
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.
We use snake_case
for identifiers.
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.
A comment explaining why we do this ±3 would also be good.
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.
After more testing, and thinking about it more.... I honestly don't think it's a good idea. It feels like it works nicely when you have the knowledge of how it is supposed to work, but if you don't know that it's based on the cursor position it might feel like a bug. For me it feels like one or the other is more intuitive. Drag at mouse position feels fine. Drag at cursor feels fine, too. But if you drag at cursor, only when the mouse is near the cursor it doesn't feel right because if you're gonna position the cursor at a certain spot, you should be able to drag anywhere on the editor and not be forced to be specifically within the range of the cursor. For example, what if you're typing along and the cursor is at the end of the line... then you try dragging between the parenthesis, which is at the end of the line, too... it'll position at the very end of the line and not betweeen the parenthesis and since you don't even know that it's based on the cursor, it feels clunky.
As you said before, if there are visible cues that it snaps to the cursor, it probably would make more sense. Until then, I think this small fix more confusing. I felt like this fix would be a better alternative until implementing that, but it's worse than if we had an option for drag at cursor or at mouse position. The modifier key might actually make more sense, but then we run in to the same argument of extra key settings.
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.
@codecustard Does it feel better to use if you decrease the tolerance to 1 or 2 characters (from 3)?
Seems good apart from a couple style issues. The commit log should also be amended to properly describe what the commit now does. |
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.
Ok to merge pending style fixes.
Another option (visually) would be to have the dragged file not snap to the cursor, but instead have the cursor follow the mouse to show exactly where it will be inserted. After dropping the file (path), the text cursor could stay at the end of the inserted text, and the script area could be active (it looks like the file area ends up active currently). Also, making the mouse cursor into a pointer instead of a hand when dragging over the script area would make it even easier to position precisely. |
@codecustard Is this still desired? If so, it needs to be rebased on the latest master branch. While there are no conflicts, rebasing is important so that reviewers can easily test the PR. If not, abandoned pull requests will be closed in the future as announced here. |
Closing as it still needs some changes, and from the last comments it was not clear if the final implementation was actually a good improvement. Might need some more testing/discussion to figure out how to have the best UX for this operation. |
Closes: #28472