-
Notifications
You must be signed in to change notification settings - Fork 292
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
[REFACTORING] Improve Hedy Editor interface #4435
Changes from 13 commits
fe22b9d
722204e
5003613
140e261
ff4bf73
7b09385
e8c6e75
8685728
646be8e
b788e6e
88ced94
e87eb15
a4d6c85
a20235d
683f54c
9eca1af
2088465
ef23c71
515f2d7
be5b01d
1d38739
10e0da5
a9e379f
5f86852
0165ffb
58719e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,9 @@ import { localDelete, localLoad, localSave } from './local'; | |
import { initializeLoginLinks } from './auth'; | ||
import { postJson } from './comm'; | ||
import { LocalSaveWarning } from './local-save-warning'; | ||
import { HedyEditor } from './editor'; | ||
import { HedyEditor, EditorType } from './editor'; | ||
import { HedyAceEditorCreator } from './ace-editor'; | ||
import { stopDebug } from "./debugging"; | ||
|
||
export let theGlobalEditor: HedyEditor; | ||
export let theModalEditor: HedyEditor; | ||
|
@@ -184,7 +185,11 @@ export function initializeCodePage(options: InitializeCodePageOptions) { | |
// *** EDITOR SETUP *** | ||
const $editor = $('#editor'); | ||
if ($editor.length) { | ||
theGlobalEditor = editorCreator.initializeMainEditor($editor); | ||
const dir = $("body").attr("dir"); | ||
theGlobalEditor = editorCreator.initializeWritableEditor($editor, EditorType.MAIN, dir); | ||
attachMainEditorEvents(); | ||
error.setEditor(theGlobalEditor); | ||
window.Range = ace.require('ace/range').Range // get reference to ace/range | ||
} | ||
|
||
const anchor = window.location.hash.substring(1); | ||
|
@@ -239,6 +244,79 @@ export function initializeCodePage(options: InitializeCodePageOptions) { | |
$('#program_name').on('blur', () => saveIfNecessary()); | ||
} | ||
|
||
function attachMainEditorEvents() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might pass the editor in as an argument, and get rid of at least one use of the global variable? |
||
|
||
theGlobalEditor.on('change', () => { | ||
theLocalSaveWarning.setProgramLength(theGlobalEditor.contents.split('\n').length); | ||
// theGlobalEditor.markers.clearIncorrectLines(); => part of skip faulty feauture | ||
}); | ||
|
||
// If prompt is shown and user enters text in the editor, hide the prompt. | ||
theGlobalEditor.on('change', function() { | ||
if (askPromptOpen) { | ||
stopit(); | ||
theGlobalEditor.focus(); // Make sure the editor has focus, so we can continue typing | ||
} | ||
if ($('#ask-modal').is(':visible')) $('#inline-modal').hide(); | ||
askPromptOpen = false; | ||
$('#runit').css('background-color', ''); | ||
theGlobalEditor.clearErrors(); | ||
//removing the debugging state when loading in the editor | ||
stopDebug(); | ||
}); | ||
|
||
// *** KEYBOARD SHORTCUTS *** | ||
|
||
let altPressed: boolean | undefined; | ||
// alt is 18, enter is 13 | ||
window.addEventListener ('keydown', function (ev) { | ||
const keyCode = ev.keyCode; | ||
if (keyCode === 18) { | ||
altPressed = true; | ||
return; | ||
} | ||
if (keyCode === 13 && altPressed) { | ||
if (!theLevel || !theLanguage) { | ||
throw new Error('Oh no'); | ||
} | ||
runit (theLevel, theLanguage, "", function () { | ||
$ ('#output').focus (); | ||
}); | ||
} | ||
// We don't use jquery because it doesn't return true for this equality check. | ||
if (keyCode === 37 && document.activeElement === document.getElementById ('output')) { | ||
theGlobalEditor.focus(); | ||
theGlobalEditor.moveCursorToEndOfFile(); | ||
} | ||
}); | ||
window.addEventListener ('keyup', function (ev) { | ||
triggerAutomaticSave(); | ||
const keyCode = ev.keyCode; | ||
if (keyCode === 18) { | ||
altPressed = false; | ||
return; | ||
} | ||
}); | ||
|
||
// Removed until we can fix the skip lines feature | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohh, this is the "debugger", isn't it? What's the issue with it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is the skip line feature. It had a weird problem where if 2 different programs in different sessions where being executed at the same time and both had errors, they had their errors mixed up. We already have a fix for this in #4414, but we decided to wait a few weeks and keep testing before shipping it. |
||
// We show the error message when clicking on the skipped code | ||
// this._editor.on("click", function(e) { | ||
// let position = e.getDocumentPosition() | ||
// position = e.editor.renderer.textToScreenCoordinates(position.row, position.column) | ||
|
||
// let element = document.elementFromPoint(position.pageX, position.pageY) | ||
// if (element !== null && element.className.includes("ace_incorrect_hedy_code")){ | ||
// let mapIndex = element.classList[0].replace('ace_incorrect_hedy_code_', ''); | ||
// let mapError = theGlobalSourcemap[mapIndex]; | ||
|
||
// $('#okbox').hide (); | ||
// $('#warningbox').hide(); | ||
// $('#errorbox').hide(); | ||
// error.show(ClientMessages['Transpile_error'], mapError.error); | ||
// } | ||
// }); | ||
} | ||
|
||
export interface InitializeViewProgramPageOptions { | ||
readonly page: 'view-program'; | ||
readonly level: number; | ||
|
@@ -250,7 +328,11 @@ export function initializeViewProgramPage(options: InitializeViewProgramPageOpti | |
theLanguage = options.lang; | ||
|
||
// We need to enable the main editor for the program page as well | ||
theGlobalEditor = editorCreator.initializeMainEditor($('#editor')); | ||
const dir = $("body").attr("dir"); | ||
theGlobalEditor = editorCreator.initializeWritableEditor($('#editor'), EditorType.MAIN, dir); | ||
attachMainEditorEvents(); | ||
error.setEditor(theGlobalEditor); | ||
window.Range = ace.require('ace/range').Range // get reference to ace/range | ||
} | ||
|
||
export function initializeHighlightedCodeBlocks(where: Element) { | ||
|
@@ -269,9 +351,9 @@ export function initializeHighlightedCodeBlocks(where: Element) { | |
// Otherwise, the teacher manual Frequent Mistakes page is SUPER SLOW to load. | ||
onElementBecomesVisible(preview, () => { | ||
// Create this example editor | ||
const exampleEditor = editorCreator.initializeExampleEditor(preview) | ||
const exampleEditor = editorCreator.initializeReadOnlyEditor(preview, dir); | ||
// Strip trailing newline, it renders better | ||
exampleEditor.setValue(exampleEditor.getValue().trimRight()); | ||
exampleEditor.contents = exampleEditor.contents.trimRight(); | ||
// And add an overlay button to the editor if requested via a show-copy-button class, either | ||
// on the <pre> itself OR on the element that has the '.turn-pre-into-ace' class. | ||
if ($(preview).hasClass('show-copy-button') || $(container).hasClass('show-copy-button')) { | ||
|
@@ -281,8 +363,8 @@ export function initializeHighlightedCodeBlocks(where: Element) { | |
symbol = "⇤"; | ||
} | ||
$('<button>').css({ fontFamily: 'sans-serif' }).addClass('yellow-btn').text(symbol).appendTo(buttonContainer).click(function() { | ||
if (!theGlobalEditor?.isReadOnly()) { | ||
theGlobalEditor?.setValue(exampleEditor.getValue() + '\n'); | ||
if (!theGlobalEditor?.isReadOnly) { | ||
theGlobalEditor.contents = exampleEditor.contents + '\n'; | ||
} | ||
update_view("main_editor_keyword_selector", <string>$(preview).attr('lang')); | ||
stopit(); | ||
|
@@ -292,7 +374,7 @@ export function initializeHighlightedCodeBlocks(where: Element) { | |
|
||
const levelStr = $(preview).attr('level'); | ||
if (levelStr) { | ||
exampleEditor.setHighliterForLevel(parseInt(levelStr, 10)); | ||
exampleEditor.setHighlighterForLevel(parseInt(levelStr, 10)); | ||
} | ||
}); | ||
} | ||
|
@@ -582,11 +664,11 @@ function removeBulb(){ | |
* Called when the user clicks the "Try" button in one of the palette buttons | ||
*/ | ||
export function tryPaletteCode(exampleCode: string) { | ||
if (theGlobalEditor?.isReadOnly()) { | ||
if (theGlobalEditor?.isReadOnly) { | ||
return; | ||
} | ||
|
||
theGlobalEditor.setValue(exampleCode + '\n'); | ||
theGlobalEditor.contents = exampleCode + '\n'; | ||
//As the commands try-it buttons only contain english code -> make sure the selected language is english | ||
if (!($('#editor').attr('lang') == 'en')) { | ||
$('#editor').attr('lang', 'en'); | ||
|
@@ -1298,11 +1380,7 @@ function get_parsons_code() { | |
} | ||
|
||
export function get_active_and_trimmed_code() { | ||
|
||
theGlobalEditor.trimTrailingSpace(); | ||
const code = returnLinesWithoutBreakpoints(theGlobalEditor); | ||
|
||
return code; | ||
return returnLinesWithoutBreakpoints(theGlobalEditor); | ||
} | ||
|
||
export function confetti_cannon(){ | ||
|
@@ -1356,7 +1434,8 @@ export function modalStepOne(level: number){ | |
createModal(level); | ||
let $modalEditor = $('#modal-editor'); | ||
if ($modalEditor.length) { | ||
theModalEditor = editorCreator.initializeModalEditor($modalEditor) | ||
const dir = $("body").attr("dir"); | ||
theModalEditor = editorCreator.initializeWritableEditor($modalEditor, EditorType.MODAL, dir); | ||
} | ||
} | ||
|
||
|
@@ -1453,7 +1532,7 @@ export function change_keyword_language(start_lang: string, new_lang: string) { | |
}); | ||
|
||
if (response.success) { | ||
theGlobalEditor.setValue(response.code); | ||
theGlobalEditor.contents = response.code; | ||
$('#editor').attr('lang', new_lang); | ||
update_view('main_editor_keyword_selector', new_lang); | ||
} | ||
|
@@ -1626,7 +1705,7 @@ function updatePageElements() { | |
$('#editor').toggle(isCodeTab); | ||
$('#debug_container').toggle(isCodeTab); | ||
$('#program_name_container').toggle(isCodeTab); | ||
theGlobalEditor.setEditorMode(false); | ||
theGlobalEditor.isReadOnly = false; | ||
|
||
const adventure = theAdventures[currentTab]; | ||
if (adventure) { | ||
|
@@ -1655,7 +1734,7 @@ function updatePageElements() { | |
$('[data-view="if-submitted"]').toggle(isSubmitted); | ||
$('[data-view="if-not-submitted"]').toggle(!isSubmitted); | ||
|
||
theGlobalEditor.setEditorMode(isSubmitted); | ||
theGlobalEditor.isReadOnly = isSubmitted; | ||
} | ||
} | ||
|
||
|
@@ -1675,7 +1754,7 @@ function reconfigurePageBasedOnTab() { | |
const adventure = theAdventures[currentTab]; | ||
if (adventure) { | ||
$ ('#program_name').val(adventure.save_name); | ||
theGlobalEditor?.setValue(adventure.start_code); | ||
theGlobalEditor.contents = adventure.start_code; | ||
} | ||
} | ||
|
||
|
@@ -1774,7 +1853,7 @@ function programNeedsSaving(adventureName: string) { | |
// We need to save if the content changed, OR if we have the opportunity to | ||
// save a program that was loaded from local storage to the server. | ||
// (Submitted programs are never saved again). | ||
const programChanged = theGlobalEditor.getValue() !== adventure.start_code; | ||
const programChanged = theGlobalEditor.contents !== adventure.start_code; | ||
const nameChanged = $('#program_name').val() !== adventure.save_name; | ||
const localStorageCanBeSavedToServer = theUserIsLoggedIn && adventure.save_info === 'local-storage'; | ||
const isUnchangeable = isServerSaveInfo(adventure.save_info) ? adventure.save_info.submitted : false; | ||
|
@@ -1784,7 +1863,7 @@ function programNeedsSaving(adventureName: string) { | |
// "Run" button will always save regardless of size. | ||
const wasSavedBefore = adventure.save_info !== undefined; | ||
const suspiciouslySmallFraction = 0.5; | ||
const programSuspiciouslyShrunk = wasSavedBefore && theGlobalEditor.getValue().length < adventure.start_code.length * suspiciouslySmallFraction; | ||
const programSuspiciouslyShrunk = wasSavedBefore && theGlobalEditor.contents.length < adventure.start_code.length * suspiciouslySmallFraction; | ||
|
||
return (programChanged || nameChanged || localStorageCanBeSavedToServer) && !isUnchangeable && !programSuspiciouslyShrunk; | ||
} | ||
|
@@ -1815,7 +1894,7 @@ async function saveIfNecessary() { | |
|
||
console.info('Saving program automatically...'); | ||
|
||
const code = theGlobalEditor.getValue(); | ||
const code = theGlobalEditor.contents; | ||
const saveName = saveNameFromInput(); | ||
|
||
|
||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof 😥. I see why this might be done, but sticking it
window.Range
feels a little dirty to me.Can we not make a global:
?
At least it would be keeping out of the global browser object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weird part is that this is not being referenced anywhere, so I thought this might be a property of the
window
object. If that's not the case maybe we can remove this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window
is the object that represents the global scope.So if you do:
At any point later you can do
And it will work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. I removed the reference to windows.Range since it wasn't really used anywhere