Skip to content

Conversation

DonJayamanne
Copy link

For #9340
To be merged after #9840

Basically I got rid of the root level state info that keeps track of the cell that's selected and focused.
This required us to keep the two in sync along with the state at the cell level.
When syncing information between multiple editors this got messy. removing this removes the issues where things could go out of sync, i.e. two ways to storing the same thing. Now we get selected information from the cells directly - single source of truth.

@DonJayamanne DonJayamanne requested review from DavidKutu, IanMatthewHuff and rchiodo and removed request for rchiodo February 3, 2020 23:27
@DonJayamanne DonJayamanne added the no-changelog No news entry required label Feb 3, 2020
@codecov-io
Copy link

codecov-io commented Feb 3, 2020

Codecov Report

❗ No coverage uploaded for pull request base (ds/custom_editor@666e07d). Click here to learn what that means.
The diff coverage is 7.69%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##             ds/custom_editor    #9882   +/-   ##
===================================================
  Coverage                    ?   60.62%           
===================================================
  Files                       ?      555           
  Lines                       ?    29737           
  Branches                    ?     4482           
===================================================
  Hits                        ?    18027           
  Misses                      ?    10709           
  Partials                    ?     1001
Impacted Files Coverage Δ
src/datascience-ui/interactive-common/mainState.ts 53.57% <7.69%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 666e07d...d6a9463. Read the comment docs.

@DonJayamanne DonJayamanne reopened this Feb 3, 2020
const info: { selectedCellId?: string; selectedCellIndex?: number; focusedCellId?: string; focusedCellIndex?: number } = {};
for (let index = 0; index < state.cellVMs.length; index += 1) {
const cell = state.cellVMs[index];
if (cell.selected) {
Copy link
Member

Choose a reason for hiding this comment

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

Focus turns on selected as well. And they should never be different so you could simplify this a bit (no need to check for focuses after finding selected, just check if that cell is also focused). NBD if you don't want to change since this doesn't matter much.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, however I'd prefer if we had a single bitwise or enum flag instead.
Right now having two flags indicates they are mutually exclusive. This could lead to funky code.

I'd prefer to do that as a separate PR.

Focus turns on selected as well

Though the styles are different in the cell, e.g. when a code cell has focus, the style is not that of a selected cell (not blue) but the striped green.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 4, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@DonJayamanne DonJayamanne merged commit 05bf2ea into microsoft:ds/custom_editor Feb 4, 2020
@DonJayamanne DonJayamanne deleted the removeRootLevelVars branch February 4, 2020 01:50
@lock lock bot locked as resolved and limited conversation to collaborators Feb 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants