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

Literals and operators in Component Browser #12420

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 38 additions & 27 deletions app/gui/src/project-view/components/ComponentBrowser.vue
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { componentBrowserBindings, listBindings } from '@/bindings'
import { type Component } from '@/components/ComponentBrowser/component'
import ComponentEditor from '@/components/ComponentBrowser/ComponentEditor.vue'
import ComponentList from '@/components/ComponentBrowser/ComponentList.vue'
import { Filtering } from '@/components/ComponentBrowser/filtering'
import { useComponentBrowserInput, type Usage } from '@/components/ComponentBrowser/input'
import GraphVisualization from '@/components/GraphEditor/GraphVisualization.vue'
import SvgButton from '@/components/SvgButton.vue'
Expand All @@ -15,7 +14,6 @@ import { injectNodeColors } from '@/providers/graphNodeColors'
import { injectInteractionHandler, type Interaction } from '@/providers/interactionHandler'
import { useGraphStore } from '@/stores/graph'
import type { RequiredImport } from '@/stores/graph/imports'
import { useProjectStore } from '@/stores/project'
import { injectProjectNames } from '@/stores/projectNames'
import { useSuggestionDbStore } from '@/stores/suggestionDatabase'
import { type Typename } from '@/stores/suggestionDatabase/entry'
Expand All @@ -29,6 +27,8 @@ import { debouncedGetter } from '@/util/reactivity'
import type { ComponentInstance } from 'vue'
import { computed, onMounted, onUnmounted, ref, toValue, watch, watchEffect } from 'vue'
import type { SuggestionId } from 'ydoc-shared/languageServerTypes/suggestions'
import { Range } from 'ydoc-shared/util/data/range'
import { Ok } from 'ydoc-shared/util/data/result'
import type { VisualizationIdentifier } from 'ydoc-shared/yjsModel'

// Difference in position between the component browser and a node for the input of the component browser to
Expand All @@ -47,7 +47,6 @@ const EDGE_Y_OFFSET = -8

const cssComponentEditorPadding = `${COMPONENT_EDITOR_PADDING}px`

const projectStore = useProjectStore()
const suggestionDbStore = useSuggestionDbStore()
const graphStore = useGraphStore()
const interaction = injectInteractionHandler()
Expand Down Expand Up @@ -177,15 +176,6 @@ const selectedSuggestion = computed(() => {

const input = useComponentBrowserInput()

const currentFiltering = computed(() => {
if (input.mode.mode === 'componentBrowsing') {
const currentModule = projectStore.moduleProjectPath
return new Filtering(input.mode.filter, currentModule?.ok ? currentModule.value : undefined)
} else {
return undefined
}
})

onUnmounted(() => {
graphStore.cbEditedEdge = undefined
})
Expand Down Expand Up @@ -285,20 +275,37 @@ watch(

// === Accepting Entry ===

function acceptSuggestion(component: Opt<Component> = null) {
const suggestionId = component?.suggestionId ?? selectedSuggestionId.value
if (suggestionId == null) return acceptInput()
const result = input.applySuggestion(suggestionId)
function applyComponent(component: Opt<Component> = null) {
component ??= selected.value
if (component == null) {
input.switchToCodeEditMode()
return Ok()
}
if (component.suggestionId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestionId is a number, and AFAIK the protocol allows it to be 0.

return input.applySuggestion(component.suggestionId)
} else {
// Component without suggestion database entry, for example "literal" component.
input.content = { text: component.label, selection: Range.emptyAt(component.label.length) }
input.switchToCodeEditMode()
return Ok()
}
}

function acceptComponent(component: Opt<Component> = null) {
const result = applyComponent(component)
if (result.ok) acceptInput()
else result.error.log('Cannot apply suggestion')
}

function applySuggestion(component: Opt<Component> = null) {
const suggestionId = component?.suggestionId ?? selectedSuggestionId.value
if (suggestionId == null) return input.switchToCodeEditMode()
const result = input.applySuggestion(suggestionId)
if (!result.ok) result.error.log('Cannot apply suggestion')
}
// function editComponent(component: Opt<Component> = null) {
// const result = applyComponentToInput(component)
// if (result.ok) acceptInput()
// else result.error.log('Cannot apply suggestion')
// const suggestionId = component?.suggestionId ?? selectedSuggestionId.value
// if (suggestionId == null) return input.switchToCodeEditMode()
// const result = input.applySuggestion(suggestionId)
// if (!result.ok) result.error.log('Cannot apply suggestion')
// }

function acceptInput() {
const appliedReturnType =
Expand All @@ -314,11 +321,14 @@ function acceptInput() {
const outsideComponentBrowsing = computed(() => input.mode.mode != 'componentBrowsing')
const actions = registerHandlers({
'componentBrowser.editSuggestion': {
action: applySuggestion,
action: () => {
const result = applyComponent()
if (!result.ok) result.error.log('Cannot apply component')
},
disabled: outsideComponentBrowsing,
},
'componentBrowser.acceptSuggestion': {
action: acceptSuggestion,
action: acceptComponent,
disabled: outsideComponentBrowsing,
},
'componentBrowser.acceptInputAsCode': {
Expand Down Expand Up @@ -427,10 +437,11 @@ const listsHandler = listBindings.handler({
/>
</div>
<ComponentList
v-if="input.mode.mode === 'componentBrowsing' && currentFiltering"
v-if="input.mode.mode === 'componentBrowsing'"
ref="componentList"
:filtering="currentFiltering"
@acceptSuggestion="acceptSuggestion($event)"
:filter="input.mode.filter"
:literal="input.mode.literal"
@acceptSuggestion="acceptComponent($event)"
@update:selectedComponent="selected = $event"
/>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,27 +1,32 @@
<script setup lang="ts">
import { makeComponentList, type Component } from '@/components/ComponentBrowser/component'
import { makeComponentLists, type Component } from '@/components/ComponentBrowser/component'
import ComponentEntry from '@/components/ComponentBrowser/ComponentEntry.vue'
import type { Filtering } from '@/components/ComponentBrowser/filtering'
import { Filter, Filtering } from '@/components/ComponentBrowser/filtering'
import SvgIcon from '@/components/SvgIcon.vue'
import VirtualizedList from '@/components/VirtualizedList.vue'
import { groupColorStyle } from '@/composables/nodeColors'
import { useProjectStore } from '@/stores/project'
import { useSuggestionDbStore } from '@/stores/suggestionDatabase'
import { Ast } from '@/util/ast'
import { tryGetIndex } from '@/util/data/array'
import { computed, ref, toRef, watch } from 'vue'
import * as map from 'lib0/map'
import { computed, ref, watch } from 'vue'
import type { ComponentExposed } from 'vue-component-type-helpers'

const ITEM_SIZE = 24
const SCROLL_TO_SELECTION_MARGIN = ITEM_SIZE / 2
const MOUSE_SELECTION_DEBOUNCE = 200

const props = defineProps<{
filtering: Filtering
filter: Filter
literal?: Ast.TextLiteral | Ast.NumericLiteral | undefined
}>()
const emit = defineEmits<{
acceptSuggestion: [suggestion: Component]
'update:selectedComponent': [selected: Component | null]
}>()

const projectStore = useProjectStore()
const root = ref<HTMLElement>()
const groupsPanel = ref<ComponentExposed<typeof VirtualizedList>>()
const componentsPanel = ref<ComponentExposed<typeof VirtualizedList>>()
Expand All @@ -42,18 +47,34 @@ const displayedSelectedComponentIndex = computed({
},
})

watch(toRef(props, 'filtering'), () => (displayedSelectedComponentIndex.value = 0))
const filtering = computed(() => {
const currentModule = projectStore.moduleProjectPath
return new Filtering(props.filter, currentModule?.ok ? currentModule.value : undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a little simpler with unwrapOr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currentModule may be undefined, so I wouldn't avoid ?:

})

watch(filtering, () => (displayedSelectedComponentIndex.value = 0))
watch(selectedGroupIndex, () => (selectedComponentIndex.value = 0))

const suggestionDbStore = useSuggestionDbStore()
const components = computed(() => makeComponentList(suggestionDbStore.entries, props.filtering))
const components = computed(() => {
const lists = makeComponentLists(suggestionDbStore.entries, filtering.value)
if (props.literal != null) {
map
.setIfUndefined(lists, 'all', (): Component[] => [])
.unshift({
label: props.literal.code(),
icon: props.literal instanceof Ast.TextLiteral ? 'text_input' : 'input_number',
})
}
return lists
})
const currentGroups = computed(() => {
return Array.from(components.value.entries(), ([id, components]) => ({
id,
...(id === 'all' ? { name: 'all' }
: id === 'suggestions' ? { name: 'suggestions' }
: (suggestionDbStore.groups[id] ?? { name: 'unknown' })),
...(props.filtering.pattern != null ? { displayedNumber: components.length } : {}),
...(filtering.value?.pattern != null ? { displayedNumber: components.length } : {}),
}))
})
const displayedGroupId = computed(() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ interface ComponentLabel {

/** A model of component suggestion displayed in the Component Browser. */
export interface Component extends ComponentLabel {
suggestionId: SuggestionId
suggestionId?: SuggestionId
icon: Icon
group?: number | undefined
}
Expand Down Expand Up @@ -112,7 +112,7 @@ export function makeComponent({ id, entry, match }: ComponentInfo): Component {
}

/** Create {@link Component} list for each displayed group from filtered suggestions. */
export function makeComponentList(
export function makeComponentLists(
db: SuggestionDb,
filtering: Filtering,
): Map<GroupId, Component[]> {
Expand Down
33 changes: 25 additions & 8 deletions app/gui/src/project-view/components/ComponentBrowser/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
type SuggestionEntry,
type SuggestionId,
} from '@/stores/suggestionDatabase/entry'
import { isIdentifier, type AstId, type Identifier } from '@/util/ast/abstract'
import { Ast } from '@/util/ast'
import { Err, Ok, type Result } from '@/util/data/result'
import { type ProjectPath } from '@/util/projectPath'
import { qnJoin, qnLastSegment } from '@/util/qualifiedName'
Expand All @@ -19,7 +19,7 @@ import { Range } from 'ydoc-shared/util/data/range'

/** Information how the component browser is used, needed for proper input initializing. */
export type Usage =
| { type: 'newNode'; sourcePort?: AstId | undefined }
| { type: 'newNode'; sourcePort?: Ast.AstId | undefined }
| { type: 'editNode'; node: NodeId; cursorPos: number }

/**
Expand All @@ -32,6 +32,7 @@ export type ComponentBrowserMode =
| {
mode: 'componentBrowsing'
filter: Filter
literal?: Ast.TextLiteral | Ast.NumericLiteral
}
| {
mode: 'codeEditing'
Expand All @@ -55,7 +56,7 @@ export function useComponentBrowserInput(
const imports = shallowRef<RequiredImport[]>([])
const processingAIPrompt = ref(false)
const toastError = useToast.error()
const sourceNodeIdentifier = ref<Identifier>()
const sourceNodeIdentifier = ref<Ast.Identifier>()
const switchedToCodeMode = ref<{ appliedSuggestion?: SuggestionEntry }>()

// Text Model to being edited externally (by user).
Expand Down Expand Up @@ -110,12 +111,20 @@ export function useComponentBrowserInput(
: {}),
}
} else {
let literal: Ast.MutableTextLiteral | Ast.NumericLiteral | undefined =
Ast.TextLiteral.tryParse(text.value)
if (literal == null) {
literal = Ast.NumericLiteral.tryParse(text.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a tryParseWithSign allowing negative numbers. This logic could use unit test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unit tests were once completely removed for input.ts, because CB got huge redesign and we didn't know if it will be stable. For now, I just (re) added tests for cases fixed by this PR.

} else {
literal.fixBoundaries()
}
return {
mode: 'componentBrowsing',
filter: {
pattern: text.value,
...(sourceNodeType.value != null ? { selfArg: sourceNodeType.value } : {}),
},
...(literal ? { literal } : {}),
}
}
})
Expand Down Expand Up @@ -172,7 +181,7 @@ export function useComponentBrowserInput(
qnJoin(
owner.path ? qnLastSegment(owner.path)
: owner.project ? qnLastSegment(owner.project)
: ('Main' as Identifier),
: ('Main' as Ast.Identifier),
entry.name,
)
: entry.name) + ' ',
Expand All @@ -192,7 +201,9 @@ export function useComponentBrowserInput(
const alreadyAdded = finalImports.some((existing) => requiredImportEquals(existing, anImport))
const importedIdent =
anImport.kind == 'Qualified' ?
qnLastSegment(anImport.module.path ?? anImport.module.project ?? ('Main' as Identifier))
qnLastSegment(
anImport.module.path ?? anImport.module.project ?? ('Main' as Ast.Identifier),
)
: anImport.import
const noLongerNeeded = !text.value.includes(importedIdent)
if (!noLongerNeeded && !alreadyAdded) {
Expand All @@ -207,7 +218,7 @@ export function useComponentBrowserInput(
case 'newNode':
if (usage.sourcePort) {
const ident = graphDb.getOutputPortIdentifier(usage.sourcePort)
sourceNodeIdentifier.value = ident != null && isIdentifier(ident) ? ident : undefined
sourceNodeIdentifier.value = ident != null && Ast.isIdentifier(ident) ? ident : undefined
} else {
sourceNodeIdentifier.value = undefined
}
Expand All @@ -234,7 +245,7 @@ export function useComponentBrowserInput(
const matchedCode = sourceNodeMatch?.[2]
if (
matchedSource != null &&
isIdentifier(matchedSource) &&
Ast.isIdentifier(matchedSource) &&
matchedCode != null &&
graphDb.getIdentDefiningNode(matchedSource)
)
Expand Down Expand Up @@ -271,7 +282,13 @@ export function useComponentBrowserInput(
}

function applySourceNode(text: string) {
return sourceNodeIdentifier.value ? `${sourceNodeIdentifier.value}.${text}` : text
return (
sourceNodeIdentifier.value ?
/^[a-zA-Z]/.test(text) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically an identifier can start with a character other than ASCII letters; we could handle that by exposing to WASM an entry point to the lexer so that we can see if the first token is a (lexical) identifier.

I suppose this is ok for an initial implementation but let's add a TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, implementation in rust should be quite easy; I just assumed this is too simple case for it.

But here you pointed me that it's not that simple case.

`${sourceNodeIdentifier.value}.${text}`
: `${sourceNodeIdentifier.value} ${text}`
: text
)
}

return proxyRefs({
Expand Down
19 changes: 18 additions & 1 deletion app/rust-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,23 @@ pub fn is_ident_or_operator(code: &str) -> u32 {
}
}

#[wasm_bindgen]
pub fn is_ident_or_operator(code: &str) -> u32 {
let parsed = enso_parser::lexer::run(code);
if parsed.internal_error.is_some() {
return 0;
}
let token = match &parsed.value[..] {
[token] => token,
_ => return 0,
};
match &token.variant {
enso_parser::syntax::token::Variant::Ident(_) => 1,
enso_parser::syntax::token::Variant::Operator(_) => 2,
_ => 0,
}
}

#[wasm_bindgen]
pub fn is_numeric_literal(code: &str) -> bool {
let parsed = PARSER.with(|parser| parser.parse_block(code));
Expand Down Expand Up @@ -74,7 +91,7 @@ mod tests {
use super::*;

#[test]
fn test_is_ident_or_operator() {
fn test_is_numeric_literal() {
assert!(is_numeric_literal("1234"));
assert!(is_numeric_literal("-1234"));
assert!(!is_numeric_literal(""));
Expand Down
10 changes: 10 additions & 0 deletions app/ydoc-shared/src/ast/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1893,6 +1893,16 @@ export class MutableTextLiteral extends TextLiteral implements MutableExpression
this.fields.set('close', unspaced(Token.new(code)))
}

fixBoundaries() {
const open = this.open
const close = this.close
if (open != null && close == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the literal is multiline, close must remain null. It is multiline if open.length > 1.

This function could use some unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I guess setBoundaries also needs this logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

setBoundaries is a very low-level operation that may break the validity of the AST in various ways, we might remove it or comment it as only for use in tests.

this.fields.set('close', unspaced(Token.new(open.code())))
} else if (open == null && close != null) {
this.fields.set('open', unspaced(Token.new(close.code())))
}
}

setElements(elements: TextElement<OwnedRefs>[]) {
this.fields.set(
'elements',
Expand Down
Loading