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

Outline explorer highlighted item doesn't change when "Follow cursor position" is unchecked #14134

Open
jnsebgosselin opened this issue Nov 3, 2020 · 11 comments

Comments

@jnsebgosselin
Copy link
Member

This is a regression that was introduced by the migration of the outline explorer to LSP.

In the outline explorer, when the option Follow cursor position is unchecked, the outline explorer should highlight the lowest item in the tree corresponding to the cursor position, without expanding anything.

This is an animation that shows how this was working before the migration to LSP:

outline_explorer_highlight_working

This is an animation showing how it works after the migration to LSP.

outline_explorer_highlight_NOT_working

Versions

  • Spyder version: 4.2.0.dev0 0dfbe89
  • Python version: 3.7.6 64-bit
  • Qt version: 5.12.5
  • PyQt5 version: 5.12.3
  • Operating System: Windows 10
@ccordoba12
Copy link
Member

Thanks for reporting this regression @jnsebgosselin. You said

In the outline explorer, when the option Follow cursor position is unchecked, the outline explorer should highlight the lowest item in the tree corresponding to the cursor position, without expanding anything.

I see two problems about this:

  1. It goes against what "Follow cursor position" means. That option is quite clear: we either follow the cursor's position (and expand to show where we are placed in the Outline tree) or we don't. But trying to achieve a mix of both (just to maintain the previous UX) seems wrong to me.
  2. It seems hard to implement with the current architecture and something that could lead again to code that's very hard to maintain (the previous code was messy and @andfoy and I had to invest a lot of time untangling it).

Especially given point 1, I think we shouldn't change the current behavior.

@jnsebgosselin
Copy link
Member Author

jnsebgosselin commented Nov 3, 2020

Its your decision to make, but as a user, I disagree with you on this. However, if its too complicated or tricky to implement, than as a developer, I understand the decision.

In my opinion however, if the highlighted item in the outline explorer doesn't reflect the cursor positioning in the editor, than it is just confusing UX to have anything highlighted in the outline explorer when the option "Follow cursor position" is unchecked. Maybe we should just not highlight anything then.

What information is the user supposed to get from the highlighted item in the outline explorer if not in which file and what part of the file he is?

It was the "expand-items-automatically-to-follow-the-cursor-position" that was the problem, not the fact that the highlighted item in the outline explorer was reflecting the position of the cursor in the editor.

It goes against what "Follow cursor position" means. That option is quite clear: we either follow the cursor's position (and expand to show where we are placed in the Outline tree) or we don't. But trying to achieve a mix of both (just to maintain the previous UX) seems wrong to me.

Than we just need to change the "Follow cursor position" wording or add a tooltip to that action that clarifies what it does exactly. But imo, I don't really see this as a problem, there is nuances in "Follow cursor position" that doesn't make it wrong to allow the highlighted item to reflect the position of the cursor in the editor.

Anyway, maybe this is just me and this is a non-issue. You should ask what others think about this I think.

@jnsebgosselin
Copy link
Member Author

jnsebgosselin commented Nov 3, 2020

I don't know if this can help to sort this issue out, but in the Windows file explorer tree view, there is an option that is named Expand to current folder. I think this is is very similar to the Follow cursor position option of the outline explorer.

So maybe a better name for the option Follow cursor position would be Expand to current cursor position.

Here is an animation showing what it does when the option is unchecked and when it is checked.

When the option Expand to current cursor position is unchecked:
file_explorer_not_expanding

When the option Expand to current cursor position is checked:
file_explorer_expanding

@jnsebgosselin
Copy link
Member Author

@ccordoba12 ok after paying a little bit more attention to this feature while working, if its technically too challenging to implement this feature back, it is not a big deal. I can get behind that 100%.

However, in my opinion, this still represents a UX downgrade from what we had before the migration to the pyls. The outline explorer is an awesome tool to see where you are in the editor at a glance of an eye. Now, you just need to think a little bit more than before.

If I recall correctly, I think this feature was introduced late in the 4.x development by @impact27 in PR #9219. So I was able to live without it before, I guess I will be able to live without it again.

@ccordoba12
Copy link
Member

@ccordoba12 ok after paying a little bit more attention to this feature while working, if its technically too challenging to implement this feature back, it is not a big deal. I can get behind that 100%.

Ok, leaving for 4.2.1 then because it's not critical (but I'm not making promises though).

However, in my opinion, this still represents a UX downgrade from what we had before the migration to the pyls

Sure, but the previous implementation degraded typing performance in the Editor significantly, especially for large files. That's why the move to the PyLS was a must.

@jnsebgosselin
Copy link
Member Author

jnsebgosselin commented Nov 3, 2020

Ok, leaving for 4.2.1 then because it's not critical (but I'm not making promises though).

You aren't making promises because you're not sure if you're ok with this feature from a UX perspective or because you're not sure you will be able to devote development to implement this?

@ccordoba12
Copy link
Member

Because I don't know how hard it'll be to implement.

@jnsebgosselin
Copy link
Member Author

Because I don't know how hard it'll be to implement.

Ok thank you. In that case, maybe I can help with this. I'm not very good with all the stuff that was added lately related to the pyls, but I guess I can learn.

@ccordoba12
Copy link
Member

Great! Thanks for your help!

@MCilento93
Copy link

I append my words to this posts because from the videos I see the same issue :
What is changed too with spyder 4.2.0 is that from outline lines breaker (like #---... - #) and sections (lines starting with ###) arent recognised.

Compare for example the tree in the first two videos

Sincerely,

@jnsebgosselin
Copy link
Member Author

jnsebgosselin commented Nov 25, 2020

What is changed too with spyder 4.2.0 is that from outline lines breaker (like #---... - #) and sections (lines starting with ###) arent recognised.

@MCilento93 I think this is being taken care of in spyder-ide/pyls-spyder#10

@ccordoba12 ccordoba12 modified the milestones: v4.2.1, v4.2.2 Dec 8, 2020
@ccordoba12 ccordoba12 removed this from the v4.2.2 milestone Jan 6, 2021
@ccordoba12 ccordoba12 added this to the v5.4.1 milestone Oct 11, 2022
@ccordoba12 ccordoba12 modified the milestones: v5.4.1, v5.4.2 Dec 6, 2022
@ccordoba12 ccordoba12 modified the milestones: v5.4.2, v5.4.3 Jan 5, 2023
@ccordoba12 ccordoba12 modified the milestones: v5.4.3, v6.0alpha2 Jan 20, 2023
@ccordoba12 ccordoba12 modified the milestones: v6.0alpha2, v6.0alpha3 Jun 8, 2023
@ccordoba12 ccordoba12 modified the milestones: v6.0alphaX, v6.1.0 Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants