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

CM6: Fix missing mergeview pickers #676

Closed
Tracked by #659
vidartf opened this issue Sep 18, 2023 · 6 comments · Fixed by #691
Closed
Tracked by #659

CM6: Fix missing mergeview pickers #676

vidartf opened this issue Sep 18, 2023 · 6 comments · Fixed by #691
Assignees

Comments

@vidartf
Copy link
Collaborator

vidartf commented Sep 18, 2023

After the CM6 upgrade, there is now missing picker handles in a few cases, especially for merge spacers. These should be re-added.

@vidartf
Copy link
Collaborator Author

vidartf commented Sep 18, 2023

Note, on #673, this was listed as:

chosen differences or differences related to CM6
- all padding spacers are directly displayed on the view, there is no need to click on a picker gutter markers to make a spacer fully expend with its real sizing.
- there is no picker gutter marker for spacers. In the merge editor, only the text chunks are shown, there is no spacer.

I don't really understand this, as the pickers are meant to be used to pick which version of the code to use (local, remote or base).

@krassowski
Copy link
Member

Somewhat related issue is that clicking on the gutter in a place which does not have a picker leads to an uncaught error:

Uncaught TypeError: o is undefined
    onGutterClick mergeview.ts:1415
    mousedown mergeview.ts:1110
    ar index.js:9652

@HaudinFlorence
Copy link
Contributor

HaudinFlorence commented Sep 27, 2023

Note, on #673, this was listed as:

chosen differences or differences related to CM6

  • all padding spacers are directly displayed on the view, there is no need to click on a picker gutter markers to make a spacer fully expend with its real sizing.
  • there is no picker gutter marker for spacers. In the merge editor, only the text chunks are shown, there is no spacer.

I don't really understand this, as the pickers are meant to be used to pick which version of the code to use (local, remote or base).

@vidartf On the contrary, I don't understand why one needs to click on a picker to make the spacers appear. Maybe I missed something in the logics choosen for nbdime. I thought that the spacers were there to align the different views (like having a direct visual comparison of the different versions) and were not related to a choice of what code will be picked. I am interested to please have further explanations. Thanks.

@vidartf
Copy link
Collaborator Author

vidartf commented Sep 27, 2023

@HaudinFlorence My comment was mainly about the second point "there is no picker gutter marker for spacers. In the merge editor, only the text chunks are shown, there is no spacer". My understanding is that the pickers are needed also when there is added code vs an "empty spacer", so that you can pick the version of the code without the addition. For the first point, I'm not sure what it refers to, but that was at least never the intended behavior, and would sound like an indicator of a bug.

@krassowski
Copy link
Member

My understanding is that the pickers are needed also when there is added code vs an "empty spacer", so that you can pick the version of the code without the addition.

So that should be solved by #691 :)

@HaudinFlorence
Copy link
Contributor

@HaudinFlorence My comment was mainly about the second point "there is no picker gutter marker for spacers. In the merge editor, only the text chunks are shown, there is no spacer". My understanding is that the pickers are needed also when there is added code vs an "empty spacer", so that you can pick the version of the code without the addition. For the first point, I'm not sure what it refers to, but that was at least never the intended behavior, and would sound like an indicator of a bug.

@vidartf Ok sorry for the confusion and thanks for the explanation.

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 a pull request may close this issue.

3 participants