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

Fix some todos #289

Merged
merged 9 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from 8 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
3 changes: 2 additions & 1 deletion spx-gui/.eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module.exports = {
shallowOnly: true
}
],
'no-console': ['warn', { allow: ['warn', 'error'] }]
'no-console': ['warn', { allow: ['warn', 'error'] }],
'no-redeclare': 'off'
}
}
25 changes: 23 additions & 2 deletions spx-gui/src/apis/common/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
*/

import { apiBaseUrl } from '@/utils/env'
import { Exception } from '@/utils/exception'
import { ApiException } from './exception'

export type RequestOptions = {
method: string
headers?: Headers
/** Timeout duration in milisecond, from request-sent to server-response-got */
timeout?: number
}

/** Response body when exception encountered for API calling */
Expand All @@ -25,6 +28,17 @@ function isApiExceptionPayload(body: any): body is ApiExceptionPayload {
/** AuthProvider provide value for header Authorization */
export type AuthProvider = () => Promise<string | null>

// milisecond
const defaultTimeout = 10 * 1000

class TimeoutException extends Exception {
name = 'TimeoutException'
userMessage = { en: 'request timeout', zh: '请求超时' }
constructor() {
super('request timeout')
}
}

export class Client {
private getAuth: AuthProvider = async () => null

Expand All @@ -46,7 +60,6 @@ export class Client {
}

private async handleResponse(resp: Response): Promise<unknown> {
// TODO: timeout
if (!resp.ok) {
const body = await resp.json()
if (!isApiExceptionPayload(body)) {
Expand All @@ -59,7 +72,15 @@ export class Client {

private async request(url: string, payload: unknown, options?: RequestOptions) {
const req = await this.prepareRequest(url, payload, options)
const resp = await fetch(req)
const timeout = options?.timeout ?? defaultTimeout
const ctrl = new AbortController()
const resp = await Promise.race([
fetch(req, { signal: ctrl.signal }),
new Promise<never>((_, reject) => setTimeout(() => reject(new TimeoutException()), timeout))
]).catch((e) => {
if (e instanceof TimeoutException) ctrl.abort()
throw e
})
return this.handleResponse(resp)
}

Expand Down
2 changes: 0 additions & 2 deletions spx-gui/src/components/editor/EditorContextProvider.vue
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,12 @@ const props = defineProps<{

const selectedRef = ref<Selected | null>(null)

/* eslint-disable no-redeclare */ // TODO: there should be no need to configure this
function select(selected: null): void
function select(type: 'stage'): void
function select(type: 'sprite' | 'sound', name: string): void
function select(type: any, name?: string) {
selectedRef.value = name == null ? { type } : { type, name }
}
/* eslint-enable no-redeclare */

// When sprite name changed, we lose the selected state
// TODO: consider moving selected to model Project, so we can deal with renaming easily
Expand Down
43 changes: 26 additions & 17 deletions spx-gui/src/components/editor/EditorHomepage.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,20 @@
</header>
<main v-if="userStore.userInfo" class="editor-main">
<template v-if="projectName">
<EditorContextProvider v-if="project" :project="project" :user-info="userStore.userInfo">
<div v-if="isLoading" class="loading-wrapper">
<NSpin size="large" />
</div>
<div v-else-if="error != null">
{{ _t(error.userMessage) }}
</div>
<EditorContextProvider
v-else-if="project != null"
:project="project"
:user-info="userStore.userInfo"
>
<ProjectEditor />
</EditorContextProvider>
<NSpin v-else size="large" />
<div v-else>TODO</div>
</template>
<template v-else>
<ProjectList @selected="handleSelected" />
Expand All @@ -19,7 +29,7 @@
</template>

<script setup lang="ts">
import { watchEffect, ref, watch } from 'vue'
import { watchEffect, computed, watch } from 'vue'
import { useRouter } from 'vue-router'
import { NButton, NSpin } from 'naive-ui'
import type { ProjectData } from '@/apis/project'
Expand All @@ -28,10 +38,10 @@ import { Project } from '@/models/project'
import TopNav from '@/components/top-nav/TopNav.vue'
import ProjectList from '@/components/project/ProjectList.vue'
import { useCreateProject } from '@/components/project'
import ProjectEditor from './ProjectEditor.vue'
import { getProjectEditorRoute } from '@/router'
import { computed } from 'vue'
import { useQuery } from '@/utils/exception'
import EditorContextProvider from './EditorContextProvider.vue'
import ProjectEditor from './ProjectEditor.vue'

const localCacheKey = 'TODO_GOPLUS_BUILDER_CACHED_PROJECT'

Expand All @@ -46,26 +56,25 @@ watchEffect(() => {

const router = useRouter()
const createProject = useCreateProject()
const project = ref<Project | null>(null)
const projectName = computed(
() => router.currentRoute.value.params.projectName as string | undefined
)

watch(
() => projectName.value,
async (projectName) => {
if (userStore.userInfo == null) return
if (projectName == null) {
project.value = null
return
}
const {
data: project,
isFetching: isLoading,
error
} = useQuery(
async () => {
if (userStore.userInfo == null) return null
if (projectName.value == null) return null
// TODO: UI logic to handle conflicts when there are local cache
const newProject = new Project()
await newProject.loadFromCloud(userStore.userInfo.name, projectName)
await newProject.loadFromCloud(userStore.userInfo.name, projectName.value)
newProject.syncToLocalCache(localCacheKey)
project.value = newProject
return newProject
},
{ immediate: true }
{ en: 'Load project failed', zh: '加载项目失败' }
)

watch(
Expand Down
11 changes: 3 additions & 8 deletions spx-gui/src/components/editor/panels/sprite/SpriteItem.vue
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
</template>

<script setup lang="ts">
import { ref, effect, computed } from 'vue'
import { computed } from 'vue'
import { useFileUrl } from '@/utils/file'
import { Sprite } from '@/models/sprite'

const props = defineProps<{
Expand All @@ -19,13 +20,7 @@ const emit = defineEmits<{
remove: []
}>()

const imgSrc = ref<string | null>(null)

effect(async () => {
const img = props.sprite.costume?.img
imgSrc.value = img != null ? await img.url() : null // TODO: race condition
})

const imgSrc = useFileUrl(() => props.sprite.costume?.img)
const imgStyle = computed(() => imgSrc.value && { backgroundImage: `url("${imgSrc.value}")` })
</script>

Expand Down
14 changes: 3 additions & 11 deletions spx-gui/src/components/editor/panels/stage/StagePanel.vue
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
</template>

<script setup lang="ts">
import { computed, ref, effect } from 'vue'
import { computed } from 'vue'
import { NDropdown, NButton } from 'naive-ui'
import { useAddAssetFromLibrary, useAddAssetToLibrary } from '@/components/library'
import { useMessageHandle } from '@/utils/exception'
import { useI18n } from '@/utils/i18n'
import { AssetType } from '@/apis/asset'
import { selectImg } from '@/utils/file'
import { selectImg, useFileUrl } from '@/utils/file'
import { fromNativeFile } from '@/models/common/file'
import { Backdrop } from '@/models/backdrop'
import { stripExt } from '@/utils/path'
Expand All @@ -40,15 +40,7 @@ function select() {
}

const backdrop = computed(() => editorCtx.project.stage.backdrop)

// TODO: we may need a special img component for [File](src/models/common/file.ts)
const imgSrc = ref<string | null>(null)

effect(async () => {
const img = backdrop.value?.img
imgSrc.value = img != null ? await img.url() : null // TODO: race condition
})

const imgSrc = useFileUrl(() => backdrop.value?.img)
const imgStyle = computed(() => imgSrc.value && { backgroundImage: `url("${imgSrc.value}")` })

const handleUpload = useMessageHandle(
Expand Down
2 changes: 1 addition & 1 deletion spx-gui/src/components/editor/panels/todo/AssetAddBtn.vue
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ const beforeUpload = async (
if (uploadFile.file != null) {
const file = await fromNativeFile(uploadFile.file)
let fileName = uploadFile.name
let assetName = stripExt(fileName) // TODO: naming conflict
let assetName = stripExt(fileName)

switch (fileType) {
case 'backdrop': {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
</v-layer>
</template>
<script setup lang="ts">
import { defineProps, watch, ref } from 'vue'
import { defineProps } from 'vue'
import { useImgFile } from '@/utils/file'
import type { Stage } from '@/models/stage'
import type { Size } from '@/models/common'

Expand All @@ -35,18 +36,5 @@ const props = defineProps<{
stage: Stage
}>()

const image = ref<HTMLImageElement>()

watch(
() => props.stage.backdrop?.img,
async (backdropImg) => {
image.value?.remove()
if (backdropImg != null) {
const _image = new window.Image()
_image.src = await backdropImg.url()
image.value = _image
}
},
{ immediate: true }
)
const image = useImgFile(() => props.stage.backdrop?.img)
</script>
22 changes: 3 additions & 19 deletions spx-gui/src/components/editor/preview/stage-viewer/Costume.vue
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import type { Sprite } from '@/models/sprite'
import type { SpriteDragMoveEvent, SpriteApperanceChangeEvent } from './common'
import type { Rect } from 'konva/lib/shapes/Rect'
import type { Size } from '@/models/common'
import { useImgFile } from '@/utils/file'

// ----------props & emit------------------------------------
const props = defineProps<{
sprite: Sprite
Expand All @@ -57,7 +59,7 @@ const displayScale = computed(
)

// ----------data related -----------------------------------
const image = ref<HTMLImageElement>()
const image = useImgFile(() => currentCostume.value?.img)
const costume = ref()
// ----------computed properties-----------------------------
// Computed spx's sprite position to konva's relative position by about changing sprite postion
Expand Down Expand Up @@ -88,24 +90,6 @@ watch(
}
)

watch(
() => currentCostume.value,
async (new_costume) => {
if (new_costume != null) {
const _image = new window.Image()
_image.src = await new_costume.img.url()
_image.onload = () => {
image.value = _image
}
} else {
image.value?.remove()
}
},
{
immediate: true
}
)

// ----------methods-----------------------------------------
/**
* @description: map spx's sprite position to konva's relative position
Expand Down
3 changes: 2 additions & 1 deletion spx-gui/src/components/project/ProjectCreate.vue
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ async function validateName(name: string): Promise<ValidationResult> {
}

// check naming conflict
const username = userStore.userInfo!.name // TODO: remove `!` here
if (userStore.userInfo == null) throw new Error('login required')
const username = userStore.userInfo.name
const existedProject = await getProject(username, name).catch((e) => {
if (e instanceof ApiException && e.code === ApiExceptionCode.errorNotFound) return null
throw e
Expand Down
10 changes: 5 additions & 5 deletions spx-gui/src/models/common/cloud.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ function parseProjectData({ files: fileUrls, ...metadata }: ProjectData) {

export async function uploadFiles(files: Files): Promise<FileCollection> {
const fileUrls: FileCollection = {}
await Promise.all(
Object.keys(files).map(async (path) => {
// TODO: keep the files' order
fileUrls[path] = await uploadFile(files[path]!)
})
const entries = await Promise.all(
Object.keys(files).map(async (path) => [path, await uploadFile(files[path]!)] as const)
)
for (const [path, fileUrl] of entries) {
fileUrls[path] = fileUrl
}
return fileUrls
}

Expand Down
2 changes: 1 addition & 1 deletion spx-gui/src/models/common/disposable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

export type Disposer = () => void

export abstract class Disposble {
export class Disposble {
_disposers: Disposer[] = []

addDisposer(disposer: Disposer) {
Expand Down
14 changes: 11 additions & 3 deletions spx-gui/src/models/common/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

import { getMimeFromExt } from '@/utils/file'
import { extname } from '@/utils/path'
import type { Disposer } from './disposable'
import { Cancelled } from '@/utils/exception'

export type Options = {
/** MIME type of file */
Expand Down Expand Up @@ -40,10 +42,16 @@ export class File {
}))
}

// TODO: remember to do URL.revokeObjectURL
async url() {
async url(onCleanup: (disposer: Disposer) => void) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

考虑不要暴露url(),让用户使用URL.createObjectURL自己创建和销毁,可以用useFileUrl(file): url包装watchEffect来监听文件改动?另外,Blob中的type是必须的,之前遇到过音频无法播放/下载扩展名错误的问题。

Copy link
Collaborator

Choose a reason for hiding this comment

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

手动包装一个Disposer有些多余

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

考虑不要暴露url(),让用户使用URL.createObjectURL自己创建和销毁

这个是考虑将来 url() 有可能不返回 object url,而是返回 file 本身的 public url(file loaded from public url 可以视作一种特殊的 lazy file,它知道文件内容对应的 public url),所以把获得 URL 的逻辑做在 File 上

另外,Blob中的type是必须的,之前遇到过音频无法播放/下载扩展名错误的问题。

这个没太理解,这里是希望 url() 返回 type 信息?

手动包装一个Disposer有些多余

“包装一个 Disposer”是指?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个没太理解,这里是希望 url() 返回 type 信息?

噢我明白你意思了,是指 URL.createObjectURL(new Blob([ab])) 构造 blob 的时候要传入 type?

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个是考虑将来 url() 有可能不返回 object url,而是返回 file 本身的 public url

如果要有这个逻辑应该分开?因为 public url不需要 revoke 而从blob创建的url需要。

噢我明白你意思了,是指 URL.createObjectURL(new Blob([ab])) 构造 blob 的时候要传入 type?

对的

“包装一个 Disposer”是指?

是指创建 URL 时通过传入 onCleanup 来清除 URL,如果按我说的由外部来创建/清除就不需要这样传入onCleanup的逻辑。我觉得传入onCleanup这个行为有些复杂/不别要

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

只是“声明一份数据使用的生命周期”,这个好像没有那么简洁的形式

将来借助 https://github.com/tc39/proposal-explicit-resource-management 这个也是一个可能的思路,它对使用者来说是简洁的;不过那个东西我没怎么用过还,跟 vue/react 结合使用会不会有坑还不清楚

Copy link
Collaborator

Choose a reason for hiding this comment

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

看了下getSize这个方法,应该还是可以在resolve之后手动调用revoke清理url,和这个比较像:

const playAudio = (asset: ExportedScratchFile) => {
  const blob = new Blob([asset.arrayBuffer], { type: getMimeFromExt(asset.extension) })
  const url = URL.createObjectURL(blob)
  let audio = new Audio(url)
  audio.play()
  audio.onended = () => {
    URL.revokeObjectURL(url)
  }
}

要求传入 onCleanup 就是在要求使用方声明 url 使用的生命周期

如果需要使用方显式地声明生命周期,这个接口从复杂度来说可能和和不封装也一样。我个人更偏向“谁创建谁销毁”这种接口形式。

Copy link
Collaborator Author

@nighca nighca Apr 10, 2024

Choose a reason for hiding this comment

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

看了下getSize这个方法,应该还是可以在resolve之后手动调用revoke清理url

当然可以由 getSize 去做,但是就像上面提到的,

外部就需要关心 createObjectURL & revokeObjectURL——后者很容易忘记,而 onCleanup 是必传的参数,可以确保不会忘记

“确保没有内存泄漏”会比接口简单更重要

这个接口从复杂度来说可能和和不封装也一样

我做在 File url() 中不是希望屏蔽 create & revoke Objecet URL 的复杂度,而是希望屏蔽 object url 跟 public url 的差异

我个人更偏向“谁创建谁销毁”这种接口形式。

File url() 去 create Object URL,也由 File url() 去 revoke,这不也符合“谁创建谁销毁”吗?

Copy link
Collaborator

Choose a reason for hiding this comment

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

File url() 去 create Object URL,也由 File url() 去 revoke,这不也符合“谁创建谁销毁”吗?

销毁的时机是又用户来决定而不是 File.url() 来决定的

而是希望屏蔽 object url 跟 public url 的差异

public url 如果也需要传入一个 cleanup 参数我认为是不太合理的

后者很容易忘记

这个应该就是 CR 需要解决的问题了

Copy link
Collaborator Author

@nighca nighca Apr 10, 2024

Choose a reason for hiding this comment

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

销毁的时机是又用户来决定而不是 File.url() 来决定的

对,用户调用 File.url() 并使用其结果,所以用户决定 File.url() cleanup 的时机,这不还是“谁创建谁销毁”吗?

public url 如果也需要传入一个 cleanup 参数我认为是不太合理的

不是 public url 需要传入 clean up,而是一份可能是 public url 的数据需要 clean up

这个应该就是 CR 需要解决的问题了

这样的问题依赖 CR 去发现才是不得已的,能让工具帮助检查当然更好

let cancelled = false
onCleanup(() => {
cancelled = true
})
const ab = await this.arrayBuffer()
return URL.createObjectURL(new Blob([ab]))
if (cancelled) throw new Cancelled()
const url = URL.createObjectURL(new Blob([ab]))
onCleanup(() => URL.revokeObjectURL(url))
return url
}
}

Expand Down
Loading
Loading