Fix markdown disappearing after editing and hitting the escape key. #8047
Conversation
| return { | ||
| id: key.toString(), | ||
| file: Uri.file(path.join(filePath, 'foo.py')).fsPath, | ||
| file: path.join(filePath, 'foo.py'), |
There was a problem hiding this comment.
Not sure why we needed this here. It was breaking the opening of the plain index.html. Assuming all the tests pass, this seems nicer. If not, I'll just lookup what the URI parse function does and duplicate that instead.
There was a problem hiding this comment.
I'm looking this up now, but I'm concerned about this. Pretty sure that Don moved everything over to URIs recently. I had to fix up a number of tests for that. I think we'll get mismatches reverting this back.
https://github.com/microsoft/vscode-python/pull/7940/files
In reply to: 336138547 [](ancestors = 336138547)
There was a problem hiding this comment.
So I like using Uri.file. I think that it's safer in the long term. But I don't like the 'req' + 'uire' hack.
In reply to: 336149686 [](ancestors = 336149686,336138547)
There was a problem hiding this comment.
Yes but if you look at the code that was there, it wasn't actually save it as a URI. It's still the file path. Seems unnecessary to me unless the path.join is returning the wrong value.
This code is only used for testing, so it should fail some test somewhere if it doesn't work.
In reply to: 336149686 [](ancestors = 336149686,336138547)
There was a problem hiding this comment.
I added that because in other parts of VS Code we end up translating paths to Uri and when we get the fsPath, it will have a leading slash as on linux. And on windows the driver letter is lower cased. Then in our tests we don't have slashes added. This results in failures in our tests.
There was a problem hiding this comment.
The require hack you had doesn't work if I open the index.html.
It fails to eval, saying eval('require') not supported.
In reply to: 336152313 [](ancestors = 336152313)
There was a problem hiding this comment.
There was a problem hiding this comment.
I know that I don't always do this, but for fixes that we think touch file paths I recommend that we queue up at least the nightly (not nightly flake) test suite on the PR. The initial URI.fspath fixes broke windows initially (presumably since Don was on Mac) then one of my followup test fixes broke linux / mac (since I was working on windows).
There was a problem hiding this comment.
So this is really about file path normalization.
Yes.
There was a problem hiding this comment.
The initial URI.fspath fixes broke windows initially (presumably since Don was on Mac) then one of my followup test fixes broke linux / mac (since I was working on windows).
Agreed. I totally forgot about the Nightly Flake tests. I tested it on the standard CI pipeline before committing this into master (I.e. it worked on all platforms).
| } | ||
|
|
||
| // Special case markdown in the edit cell. Submit it. | ||
| if (cell && cell.cell.id === Identifiers.EditCellId && cell.cell.data.cell_type === 'markdown') { |
There was a problem hiding this comment.
This case doesn't exist anymore. We don't have edit cells in the native editor.
Codecov Report
@@ Coverage Diff @@
## master #8047 +/- ##
==========================================
- Coverage 59.26% 58.95% -0.32%
==========================================
Files 499 499
Lines 22413 22413
Branches 3597 3592 -5
==========================================
- Hits 13284 13213 -71
- Misses 8300 8371 +71
Partials 829 829
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Basically I'd like to understand the failure when Uri is still used.
This should only impact the tests.
Meaning we have some test somewhere, where we're expecting file path without the leading /.
E.g. xy.py is not a valid file path on Linux, it is /xy.py and that's what Uri.file(....).fsPath gives us. Else xy.py is just a file name, not a path.
Well this code must not be happening on linux because the functional and unit tests passed on linux |
|
I can just remove this change as it's not related to the fix. I'll enter a separate issue to fix opening the index.html. |
…8047) * Fix markdown disappearing after editing and hitting the escape key. * Put back Don's hack
* Expand variable explorer tests to work for native editor too. (#7699) * Expand variable explorer tests to work for native editor too. * Add comment about async fix for editor open * Review feedback * Allow connection to servers with no token and no password (#7708) * Add capability to support current file path for notebook root (#7724) * Add capability to support current file path for notebook root * Put launch.json back * Use system variables instead of creating separate resolver * Fix mistake in config settings with passing in a uri instead of a string * Make arguments more explicit * Fix IS_WINDOWS to work on unit tests * Better cancellations (#7714) * Variable explorer and mime test fixes for tests (#7740) * Change variable explorer tests to wait for variable explorer complete * Fix native mime test * Review feedback * Try to make 2.7 tests pass * Fix liveshare guest hang issue (#7764) * initial changes * update notebooks on server session change and add test * await correctly * Fix jupyter server startup hang when xeus-cling is installed (#7773) * Make interactive window and native editor take their fontSize and fontFamily from the settings in VS Code. (#7746) * Make interactive window and native editor take their fontSize and fontFamily from the settings in VS Code. We get them in nativeEditor.tsx and in interactivePanel.tsx to apply them as styles, but we also need to send them as props to eventually assign them in the monaco editor. * Moved style from react code to common.css We now get the VS Code styles from the state of the component, instead of computing a style from an element. * Removed getting the font from nativeEditor and interactivePanel, and put it in the mainStateController * Made the interactiveWindow and the nativeEditor react and change their fonts when the user changes them in VS Code, instead of having to reload. * Changed the font props to be an object instead of two separate props to make it easier to maintain. React Context was not used to avoid making the reuse of components more difficult. * Removed some unnecesary passing around of the font prop * removed unnecesary comments * fixed a failing test * Fix a number of perf issues with large notebooks (#7778) * First idea - load cells all at once. Change updates to not update full cell Change global properties to be per cell Change cells to check for updates before rendering * Fix delete to not select at the same time * Make a pending state and a rendered state to use for rendering Some perf fixes for monaco editor. Skip laying out parent etc until necessary * Refactor intellisense to not take so much time on load * More delete and move fixes for intellisense * Fix markdown not switching Fix markdown editor background * Fix some of the time on first render * Add some more unit tests and fix intellisense issue * Add news entry and fix variable explorer open * Remove unnecessary state change * Remove unnecessary merge * Comment change * Postpone widget parent changes * Add new dependency * Update some comments and names to be clearer * Change the default cell marker (#7782) * Change the default cell marker * Fix test failures Fix gather to use new comment Fix formatting on code comments from nls * Fix unit test too * Fix gather to replace #%% * Fix selection and focus not updating when clicking around in a notebook (#7806) * Fix selection and focus not updating when clicking around in a notebook editor. * Also fix add cell to put focus in the new cell * More functional tests for the notebook editor (#7804) * New tests * Add some more functional tests for the notebook editor Make sure when an error occurs on running a cell it's displayed to the user. * Review feedback * Add native command palette cell commands and recontext IW cell commands (#7807) * Tests for keyboard shortcuts in native editor (#7797) * Tests for keyboard shortcuts * Added some more tests * Code review comments * Fix merge issues * Smoke tests does not depend on build stage (#7819) * Smoke tests does not depend on build stage * Faster CI * Tests for undo using keyboard shortcut 'z' (#7824) * Update @types/vscode (#7833) * Update vscode dependency * News entry * Fix focus problems with add new cell buttons (#7827) * Fix focus problems with add new cell buttons * Get rid of unnecessary delay for async callbacks * Use VSC API instead of hardcoding the uri resource vscode-resource (#7835) * Update vscode dependency * News entry * Stop hardcoding 'vscode-resource' and use VSC api * Remove jison dependency (#7863) * Another fix for focus on add (#7883) * Auto save native editor notebook (#7831) * Fix ctrl+s to work in the notebook editor (#7919) * Fix ctrl+s to work in the notebook editor * Fix linting error * More linter fixes * Cherry pick commit b6545e3 * Prevent updates to cell text when stauts of cell has changed (#7973) * Prevent updates to cell text when stauts of cell has changed * Added comments * When automatically opening the `Notebook Editor`, non file schemes (#7921) * Ensure we support live share * Copy *.trie files related to fontkit, as part of webpacking (#7978) * Cherry pick saving fix for ipynb files * new notebook command outside of extension (#7981) * Hide the parameters list when editor doesn't have focus (#7987) * Added comments * Fix max output size to be passed down to rendering (#8017) * Fix intellisense popping up in the wrong spot when first typing in a cell (#8018) * Fix markdown disappearing after editing and hitting the escape key. (#8047) * Fix markdown disappearing after editing and hitting the escape key. * Put back Don's hack * Fixes to allowing users to open a diff view for ipynb files (#8081) * Fixes to allowing users to open a diff view * Updated code review comments * Ensure metadata for notebooks are always present (#8109) * Ensure metadata for notebooks are always present * Let storage run in background * Refactor * Address code review * Do not auto save if notebook is untitled (#8095) * Do not auto save if notebook is untitled * Remove redundant code
4 participants
For #8045
Hitting the escape key was never actually causing a focus loss because of refactoring done around updating the render stage.