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

Add bookmarks for easier code navigation #28218

Merged
merged 1 commit into from
May 20, 2019

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Apr 20, 2019

Bookmarks!

This PR is related to issue #2746, but probably won't close it fully. In short, it adds support for code editor bookmarks. You can toggle bookmarks on specific line and jump between them easily. Bookmarks are stored per-file and not saved between sessions.

From technical side, I pretty much copy-pasted breakpoint code, so bookmarks are basically breakpoints without anything besides navigation features and with different icon. They use the same gutter, which is now shown when either of their gutters are enabled, and they don't look bad together:
image

I'm submitting this PR as draft for now, as it's the most advanced feature I added so far, so I expect it to have lots of problems before it can be merged. I guess the biggest issue is the awkward interaction between breakpoint and bookmark gutters. Is bookmark gutter even needed as an option? Maybe it should be always enabled? It doesn't make sense to disable it as it makes bookmark markers invisible. Aside from that, there are probably some interactions with e.g. shader editor I didn't properly implement (right now it's untouched). If there's anything more I missed or someone has advice for improvements, I'd really like to know.

From other notes, bookmarks occupy Ctrl + B hotkey, so I moved the old duplicate line to Ctrl + D (original shortcut didn't make sense anyways). Sadly using F2 was impossible, because it interferes with editors.

There are number of issues that might arise when this is merged, but which won't be adressed by this PR. My implementation is supposed to be base for bookmarks and it just has a few potentially missing features (which I hope will be easy to add without scrapping what I made). These are:

  • ability to see all bookmarks across opened scripts and navigate between them
  • save bookmarks for each session/file when closing editor/file //done, but only for sessions
  • toggle bookmarks with e.g. middle click in the gutter (I could actually make this one, but not sure if it's needed)
  • potentially a different bookmark icon, as it's a simple circle right now. It doesn't look bad, but I thought about adding a texture from VS Code Extension (it's MIT and looks nice).

As I said, any feedback/criticism is welcome >.>

Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Looks good!

I don't think there's a need to create a separate gutter for bookmarks, as its quite common for them to share the same space as breakpoints in other IDEs. Though I would keep the setting.

As for supporting the Shader and TextFile editor. Besides the shortcut registration, you'll have to copy the script_text_editor changes across into shader_editor_plugin and text_editor. It also might be worth extracting the _edit_option code into methods in CodeTextEditor, rather then duplicating it across each editor.

For shortcuts, I've seen Ctrl+M used a lot as 'mark' line, I don't know if that would be preferred?

I've been thinking for a while about turning breakpoints into a large dot, similar to what you've got here, so swapping to an icon would be ideal.

In addition the edit drop down is getting quite full. As a result, I would push these actions off into a "bookmark" sub-menu.

Finally, for storing state between sessions, you've already seen #27979, so it would be a matter of copying breakpoints there as well.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 1, 2019

As for supporting the Shader and TextFile editor. Besides the shortcut registration, you'll have to copy the script_text_editor changes across into shader_editor_plugin and text_editor.

I've looked into them once and there's lots of code duplication happening in there. Is there any reason why we have the same editor 3 times?

For shortcuts, I've seen Ctrl+M used a lot as 'mark' line, I don't know if that would be preferred?

M is rather far apart from Ctrl, so it's more difficult to use. And when using bookmarks, you usually want to navigate between them quickly.

swapping to an icon would be ideal.

I hope leaving it for another PR will be acceptable.

In addition the edit drop down is getting quite full. As a result, I would push these actions off into a "bookmark" sub-menu.

That menu sounds like breakpoints would fit there too. Should I move them, to make edit menu even less cluttered?

@KoBeWi
Copy link
Member Author

KoBeWi commented May 2, 2019

Ok, I implemented the bookmarks for other code editors, moved bookmarks to their own sub-menu and added the sessions stuff. I'd say the PR is ready for review. I'll squash the commits when it's reviewed/ready to merge.

@KoBeWi KoBeWi marked this pull request as ready for review May 2, 2019 12:17
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Is there any reason why we have the same editor 3 times?

Rather then re-typing, a general explanation of them can be found here: #28190 (comment)

I hope leaving it for another PR will be acceptable

Sure.

That menu sounds like breakpoints would fit there too. Should I move them, to make edit menu even less cluttered?

I wouldn't do it as part of this PR, but my bad in explaining. The bookmarks sub-menu should be placed under the edit menu, not as an entirely new menu. e.g edit --> bookmarks --> Toggle Bookmarks etc.

Finally, only one other minor issue. The bookmark_color needs to be set in editor setting _load_default_text_editor_theme method, and loaded in each of the sub-editor _load_theme_settings methods.

Otherwise, functionality wise it looks good to me.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 5, 2019

Rather then re-typing, a general explanation of them can be found here: #28190 (comment)

Still, there's loooots of code duplication happening there, which could be done better. I already created an issue for that: #28607

The bookmarks sub-menu should be placed under the edit menu, not as an entirely new menu.

Right, fixed .-.

Finally, only one other minor issue. The bookmark_color needs to be set in editor setting _load_default_text_editor_theme method, and loaded in each of the sub-editor _load_theme_settings methods.

Done.

I also squashed the commits. Hopefully it's alright now.

@akien-mga akien-mga merged commit 63e7d2d into godotengine:master May 20, 2019
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the b00km4rk5 branch May 20, 2019 21:40
@girng
Copy link

girng commented Sep 18, 2019

How I feel right now. This is a massive game changer in terms of workflow. Thanks @KoBeWi, amazing!

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

Successfully merging this pull request may close these issues.

6 participants