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.

Sign up for GitHub

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

Jump to bottom

Fix selection after running a settings quick action #2756 #3835

Closed
wants to merge 2 commits into from
Closed

Fix selection after running a settings quick action #2756 #3835

wants to merge 2 commits into from

Conversation

b-j-p
Copy link

@b-j-p b-j-p commented May 4, 2019 •
edited

Fixes #2756

Assumption: there are some quick configure actions for which a selectText may be set a priori, and then there are other quick configure actions for which this is not possible.

The change that I am proposing offers a handy way to generate selectText a posteriori, taking the whole editor into consideration, after all changes have been applied.

@b-j-p b-j-p requested a review from felixfbecker as a code owner May 4, 2019 21:47
@b-j-p b-j-p mentioned this pull request May 4, 2019
Open
@b-j-p b-j-p force-pushed the bp-fix-quick-configure branch 4 times, most recently from 1dfdcf0 to 4502b9d Compare May 9, 2019 16:59
@b-j-p
Copy link
Author

b-j-p commented May 9, 2019

@felixfbecker this fix is ready for review, when you have some time

@b-j-p b-j-p force-pushed the bp-fix-quick-configure branch 3 times, most recently from 4de0a26 to 630fcc9 Compare May 16, 2019 18:45
felixfbecker
felixfbecker reviewed May 16, 2019
View reviewed changes
Copy link
Contributor

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

I have to admit I don't have knowledge of how this code works. @sqs could you review this?

web/src/settings/MonacoSettingsEditor.tsx Outdated
monacoEdits[0].range.getStartPosition(),
monacoEdits[monacoEdits.length - 1].range.getEndPosition()
)
const editText = monacoEdits[0].text
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it okay to assume this only has one element?

Copy link
Author

@b-j-p b-j-p May 17, 2019 •
edited

Choose a reason for hiding this comment

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

editText here is a single bucket, a string consisting of all the new editions contributed to the settings editor by quick configure actions since last save update. The editions made by multiple quick configure actions (what we are interested in) will all appear together.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it's guaranteed that monacoEdits is always just one element? If so please add a comment stating that for future readers

web/src/settings/MonacoSettingsEditor.tsx Outdated
'3',
// selectText for addSlackWebhook:
'YOUR-WORKSPACE-NAME',
]
Copy link
Contributor

Choose a reason for hiding this comment

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

So we need to hard code here what the selectText is for each quick action? Why are they not defined where the quick action is defined? It's gonna be really hard to remember you need to do this in two places when adding new quick actions

Copy link
Author

@b-j-p b-j-p May 17, 2019 •
edited

Choose a reason for hiding this comment

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

We are never going to have to hard code the selectText twice. Either you can hard code it in the place where the quick configure action is defined, or you can't because of the leading comma issue. When you can't, I believe you want flexibility to set a selectText after the editions have happened. My solution offers that flexibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that the value of these strings exist twice in the code base, which means they are at risk of getting out of sync, so it is just a matter of time until they will and it will not be obvious when adding new quick actions. There must be a solution to avoid this

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean with "leading comma issue"? Are you referring to #4078 / #4008?

Copy link
Author

@b-j-p b-j-p May 23, 2019 •
edited

Choose a reason for hiding this comment

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

The leading/trailing comma issue is the issue we are addressing right now, @felixfbecker . But it sounds to me that my assumption ☝️ was incorrect. I will try an approach to this bug that is more radical, based on the assumption that a selectText for a quick configure action must always be specified in advance.

web/src/settings/MonacoSettingsEditor.tsx Outdated
monacoEdits[monacoEdits.length - 1].range.getEndPosition()
)
const editText = monacoEdits[0].text
if (editText) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's touch for me to understand what the intention is in these two branches. Comments would really help here.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for demanding this. It has made the code significantly better.

@b-j-p
Copy link
Author

b-j-p commented May 17, 2019

@felixfbecker, thanks for the feedback. Great points. I tried to respond to them with changes to the fix I am proposing. The code is much better for it. 🙏

@b-j-p b-j-p force-pushed the bp-fix-quick-configure branch 4 times, most recently from 12645ba to dd30bbc Compare May 21, 2019 23:22
@b-j-p
Copy link
Author

b-j-p commented May 28, 2019 •
edited

@felixfbecker once you revise the assumption that I began with, the present change fixes the issue, which means the bug basically disappears. All we needed were the missing selectTexts. This begs the question, why did we even care about the case in which there is no selectText specified in advance (i.e. when the type of selectText is not a string)??

If there is something I am missing, please advise. Otherwise, here you go.

@slimsag
Copy link
Member

slimsag commented Sep 11, 2019

@felixfbecker can you take another look at this soon?

@sourcegraph-bot
Copy link
Contributor

Thanks for the contribution, @b-j-p!

You should receive feedback on your pull request within a few days. If you haven't already, please read through the contributing guide, and ensure that you've signed the CLA.

Did you run into any issues when creating this PR? Please describe them in an issue so we can make the experience better for the next contributor.

1 similar comment
@sourcegraph-bot
Copy link
Contributor

Thanks for the contribution, @b-j-p!

You should receive feedback on your pull request within a few days. If you haven't already, please read through the contributing guide, and ensure that you've signed the CLA.

Did you run into any issues when creating this PR? Please describe them in an issue so we can make the experience better for the next contributor.

@nicksnyder nicksnyder mentioned this pull request Jan 28, 2020
Closed
@sourcegraph-bot
Copy link
Contributor

Thanks for the contribution, @b-j-p!

You should receive feedback on your pull request within a few days. If you haven't already, please read through the contributing guide, and ensure that you've signed the CLA.

Did you run into any issues when creating this PR? Please describe them in an issue so we can make the experience better for the next contributor.

@keegancsmith keegancsmith changed the base branch from master to main August 5, 2020 10:43
@keegancsmith keegancsmith requested a review from a team August 5, 2020 10:43
@b-j-p b-j-p closed this Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfbecker felixfbecker felixfbecker left review comments

@lguychard lguychard Awaiting requested review from lguychard

Assignees

@felixfbecker felixfbecker

Labels
new-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix selection after running a settings quick action
5 participants
@b-j-p @slimsag @sourcegraph-bot @felixfbecker @nicksnyder