Auto export snapshots #1047

gotjoshua · 2023-05-23T23:24:55Z

work in progress, very interested in feedback

i want to have snapshots show up in my logseq graph automatically... so i hacked together this mvp

@mbnuqw i really dig the architecture... very easy to dive into your code (mostly ; -)

mbnuqw · 2023-05-24T19:26:48Z

Thank you for starting work on it! It's an implementation of #876 (if I'm not mistaken), and I'll merge it when it's done.

A few first thoughts:

Permission "downloads" should be optional, but since this PR is huge by itself I can do that myself after merge
Configuration of markdown format (nested list/only headers), probably should be moved to a different PR

gotjoshua · 2023-05-24T20:56:12Z

Cool. Glad you're into it.
Yeah it's a partial solution for #876
Good call about the mark down indentation style belonging in a separate pr...

I will also need feedback about the refactoring i did... Moving some functions to the actions file that were previously in the Vue file and splitting prepare and export.

I'll also need an explanation of what happens in minify snapshot, cuz it kept me very busy trying to understand what it means for a tab to equal -1

mbnuqw · 2023-05-25T11:27:43Z

refactoring

Refactoring is ok. Cleaning up of the snapshots.vue was planned, but I always procrastinate on such tasks :)

Meaning of the "-1" values in snapshots

It means that the value is the same as in the previous snapshot (SnapStoreMode.Unchanged). Visual example:

Snapshot1	Snapshot2	Snapshot3	Normalized Snapshot3
tab1	-1	-1	tab1
tab2	tab4	-1	tab4
tab3	-1	tab5	tab5

Call the Snapshots.getNormalizedSnapshot() when you need NormalizedSnapshot. I also just found that Snapshots.getNormalizedSnapshot incorrectly handle some cases - when the previous snapshot is not found it should return undefined and we need to show error message to user (but I'll fix this later).

Also, please, use prettier: npx prettier -w src/services/snapshots.actions.ts, npx prettier -w src/page.setup/components/snapshots.vue, etc...

gotjoshua · 2023-05-25T16:31:28Z

Also, please, use prettier: npx prettier -w src/services/snapshots.actions.ts, npx prettier -w src/page.setup/components/snapshots.vue, etc...

should i take time to get this happening automatically on save via eslint? or you prefer the manual step?

also, npx prettier -w ./src/ also changes some html files that i didn't touch, do you prefer to keep them unpretty? or should i include them in the final PR?

gotjoshua · 2023-05-25T16:32:54Z

incorrectly handle some cases

ok, maybe i ran into some of those situations, but i anyway managed to work around them by calling auto export before the minification...

and many thanks for the explanations and collaboration!

mbnuqw · 2023-05-25T18:11:11Z

prettier

I meant to selectively run prettier only for changed files, but never mind, I'll setup a Github action to automate this on PR, so you don't need to run prettier manually. (I'll also update .prettierignore with html files).

and many thanks for the explanations and collaboration!

thank You for spending your time on this

This reverts commit 2c6c14e.

gotjoshua · 2023-05-26T21:24:49Z

Good call about the mark down indentation style belonging in a separate pr...

i do agree that it might have been better, but it is not so easy for me to revert those changes line by line.

How important is it for you @mbnuqw ?

Do you like the feature idea in general? (i find it really nice for pasting or importing into nested knowledge tools like Logseq)

gotjoshua · 2023-05-26T21:27:01Z

do you have any time for an interim code review?

features are basically working now, and I'll take some more time to clean up and review over the weekend.

mbnuqw · 2023-06-01T13:56:20Z

Sorry for delay.

i do agree that it might have been better, but it is not so easy for me to revert those changes line by line.
How important is it for you @mbnuqw ?

Not much, leave the features as they are.

Do you like the feature idea in general?

Yes, and it already was planned (but after v5 release), so it's good to have it implemented earlier.

Testing:

Possibly wrong indent in md output with "Tree Friendly MD (add extra bullets for windows and tabs)":

# Data/time
  - ## Window 1
  - Tab 1 (pinned)
  - Tab 2 (pinned)
    - ### Panel 1
      - Tab 3
    - ### Panel 2
      - Tab 4

It looks like Panel 1 is sub-element of the last pinned tab. What if we use this structure?:

# Data/time
- ## Window 1
  - ### Pinned tabs
    - Tab 1 (pinned)
    - Tab 2 (pinned)
  - ### Panel 1
    - Tab 3
  - ### Panel 2
    - Tab 4

Auto-export doesn't work with an empty path ("Path to Export to (relative to downloads)").

It should have the default value ("./" or maybe "./Snapshots" folder).

Firefox always asks (if configured in FF settings) where to save for auto-created snapshot.

It steals focus and will annoy users. We should use saveAs: false, in browser.downloads.download for auto-exported snapshots.

"json" and "both" format options doesn't work

mbnuqw · 2023-06-01T12:58:51Z

src/utils.ts

 const dt = new Date(ms)
 let time = `${dt.getHours()}`.padStart(2, '0')
- time += delimiter + `${dt.getMinutes()}`.padStart(2, '0')
+ if (min) time += delimiter + `${dt.getMinutes()}`.padStart(2, '0')


Remove min argument.

i'd like to keep it for a later option i have in mind (if its harmless enough for you)

mbnuqw · 2023-06-01T13:00:09Z

src/types/settings.ts

@@ -1,6 +1,7 @@
 import { SETTINGS_OPTIONS } from '../defaults'

 export interface SettingsState {
+ [x: string]: any


Remove [x: string]: any

mbnuqw · 2023-06-01T13:06:08Z

src/services/snapshots.actions.ts

 import { Containers } from './containers'
 import { DEFAULT_CONTAINER_ID } from 'src/defaults/containers'
 import { PanelType } from 'src/types/sidebar'
 import { ItemInfo } from 'src/types/tabs'
 import { Info } from './info'
 import { createDefaultSidebarConfig, getSidebarConfigFromV4 } from './sidebar-config'
+import { Favicons } from './favicons'
+import { nextTick } from 'vue'


Remove import { nextTick } from 'vue' (b/c it's unused, and I'm trying to import view lib only in components and top-lvl scripts like sidebar.ts or setup.ts)

mbnuqw · 2023-06-01T13:17:36Z

src/services/snapshots.actions.ts

 waiting = await Promise.allSettled([
 browser.storage.local.get<Stored>(['sidebar', 'containers', 'snapshots']),
- Tabs.updateBgTabsTreeData(),


Revert this change (I guess it's a debugging, but just to be sure)

I don't think i added that, rather just moved it from the old location, I'll try with and without it... And remove if not needed

Initial code:

waiting = await Promise.allSettled([ browser.storage.local.get<Stored>(['sidebar', 'containers', 'snapshots']), Tabs.updateBgTabsTreeData(), ])

PR:

console.log('updateBgTabsTreeData') await Tabs.updateBgTabsTreeData(), console.log('getting store') waiting = await Promise.allSettled([ browser.storage.local.get<Stored>(['sidebar', 'containers', 'snapshots']), ])

is this actally better in "parallel" in the allSettled array below? or ok to do it first?

mbnuqw · 2023-06-01T13:19:05Z

src/services/snapshots.actions.ts

+export async function exportSnapshot(snapshot: Snapshot) {
+ const prepared = await prepareExport(snapshot)
+ if (!prepared || !browser?.downloads)
+ return console.warn('failed attempt to export snapshot', { snapshot, prepared, browser })


Use internal Logs.warn('Snapshots.exportSnapshot: Cannot export snapshot')

mbnuqw · 2023-06-01T13:24:11Z

src/services/snapshots.actions.ts

@@ -467,7 +506,7 @@ async function adaptTabsPanels(snapshot: NormalizedSnapshot): Promise<void> {
 }

 for (const storedId of stored.sidebar.nav) {
- const storedPanel = stored.sidebar.panels[storedId]
+ const storedPanel = stored.sidebar?.panels?.[storedId]


mbnuqw · 2023-06-01T13:25:33Z

src/services/snapshots.actions.ts

+
+ let panelState = panelsById[tab.panelId]
+ if (!panelState) {
+ let panelConfig = snapshot.sidebar?.panels?.[tab.panelId]


No need for optional chaining (b/c type is NormilizedSnapshot)

mbnuqw · 2023-06-01T13:28:57Z

src/services/snapshots.actions.ts

@@ -683,7 +827,8 @@ export function convertFromV4(oldSnapshots: Snapshot_v4[]): Snapshot[] {

 // Check panel
 if (snapTab.panelId !== -1) {
- const panelConf = snapshot.sidebar.panels[snapTab.panelId]
+ const panelConf = snapshot.sidebar?.panels?.[snapTab.panelId]


No need to opt chaining b/c snapshot is NormilizedSnapshot and sidebar is SidebarConfig

mbnuqw · 2023-06-01T13:53:29Z

src/services/snapshots.actions.ts

+
+ // if(!parsed)return
+ const { id, time, containers, sidebar, tabs } = snapshot //parsed
+ const normSnapshot = { id, time, containers, sidebar, tabs }


const normSnapshot is not NormalizedSnapshot and that may cause issues in convertToMarkdown fn. It's better to use NormalizedSnapshot type instead of Snapshot | SnapshotState and update:

exportSnapshot(...) to exportSnapshot(snapshot: NormalizedSnapshot)

in createSnapshot(...) fn, pass a NormalizedSnapshot- compatible type instead of the Snapshot to the exportSnapshot():

if (Settings.state.snapAutoExport) { void exportSnapshot(currentSnapshot) }

with

if (Settings.state.snapAutoExport) { exportSnapshot({ id: currentSnapshot.id, time: currentSnapshot.time, containers: stored.containers, sidebar: stored.sidebar, tabs, }) }

onezoomin@24016a2

mbnuqw · 2023-06-01T13:55:04Z

src/services/snapshots.actions.ts

+ return { id, time, containers, sidebar, tabs, jsonFile, mdFile }
+}
+
+export async function getMostRecentSnapshot() {


getMostRecentSnapshot, getMostRecentSnapshotMd are not used, remove?

i thought i would need them, and still might if we want to decouple timing of snapshot creation and export.
i can remove them for now.

gotjoshua@9cd02a6

gotjoshua · 2023-06-01T20:05:06Z

What if we use this structure

Looks great

Thanks for the detailed review!

Looking forward to get this ready to merge...

gotjoshua · 2023-06-04T12:37:49Z

> - ## Window 1
>   - ### Pinned tabs
>     - Tab 1 (pinned)
>     - Tab 2 (pinned)
>   - ### Panel 1
>     - Tab 3

just started on this adjustment and realized that technically the pinned tabs "live" within the panels.
so when i switch panels the pinned tabs switch to the pinned tabs from the other panel

this indicates a structure like this may be better:

- ## Window 1
  - ### Panel 1 Base
    - Pinned
      - Tab 1 (pinned)
      - Tab 2 (pinned)
    - non-pinned Tab 3 ...
 ** OR **
  - ### Panel 2 - Media
    - Tab 1 (pinned)
    - Tab 2 (pinned)
    - non-pinned Tab 3 ...

notice that the pinned tabs are different for each panel:

mbnuqw · 2023-06-04T12:53:13Z

Hm... you're right, ~~it would be better to align markdown rendering with settings (pinned tabs at panel-level or at global-level - "Pinned tabs position" setting).~~

We need to change the way how snapshot is created to distinguish the global pinned tabs (e.g. with tab.panelId === -1) and per-panel (tab.panelId === "PANEL_ID"), because right now it's not possible to get the type of pinned tab. I can do this after this PR will be merged.

mbnuqw · 2023-06-04T13:52:27Z

- ## Window 1
  - ### Panel 1 Base
    - Pinned
      - Tab 1 (pinned)
      - Tab 2 (pinned)
    - non-pinned Tab 3 ...
 ** OR **
  - ### Panel 2 - Media
    - Tab 1 (pinned)
    - Tab 2 (pinned)
    - non-pinned Tab 3 ...

Both variants are ok, but the second is better b/c we don't need to translate to other languages "Pinned" (considering that the pinned tabs are marked with 📌).

gotjoshua · 2023-06-04T14:30:02Z

the second is better

Also semantically feels better to me...

gotjoshua · 2023-06-04T17:12:00Z

We need to change the way how snapshot is created to distinguish

great that you can do that after merge!

it works now (onezoomin@73b62c0)

but feels rather hackish to remerge the pinned tabs that belong in the panels.

gotjoshua · 2023-06-04T17:44:59Z

I ran out of time for this week...

if you want to finish these up, please feel free... otherwise i'll try to squeeze in some time later next week!

Auto-export doesn't work with an empty path ("Path to Export to (relative to downloads)").
- It should have the default value ("./" or maybe "./Snapshots" folder).
Firefox always asks (if configured in FF settings) where to save for auto-created snapshot.
- It steals focus and will annoy users. We should use saveAs: false, in browser.downloads.download for auto-exported snapshots.
"json" and "both" format options doesn't work

mbnuqw · 2023-06-05T11:57:15Z

Alright, thank you for your time. I'll try to finish this and publish a new release soon.

gotjoshua · 2023-06-05T17:09:05Z

I'll try to finish this

🙏

gotjoshua · 2023-06-05T17:09:23Z

I'll try to finish this

🙏

Manu [tennox] and others added 10 commits May 21, 2023 17:33

[devenv] initial setup

8e1872d

wip auto export

1adabe1

export indenting more tree friendly

4d89a90

mvp auto export

9f7e965

better collapse stucture for md

b5dfbee

setting for tree friendly md

1b16d1b

setting i18n

0a57d51

wip export type/format

8b25046

Merge remote-tracking branch 'upstream/v5' into auto-export-snapshots

471267e

panel conf

74da02b

gotjoshua added 5 commits May 25, 2023 19:14

pretty

2c6c14e

Revert "pretty"

ea27ca7

This reverts commit 2c6c14e.

Merge remote-tracking branch 'upstream/v5' into auto-export-snapshots

9acdc18

pretty (with new ignores)

df0be28

mdTree format indentation

df72d2d

mbnuqw reviewed Jun 1, 2023

View reviewed changes

pinned tabs within panels

73b62c0

gotjoshua added 5 commits June 4, 2023 18:14

remove unused fxs

7b21e62

normalization

24016a2

Merge remote-tracking branch 'upstream/v5' into auto-export-snapshots

bbb9dc4

update and getStored in parallel

5d71cc6

cleanups

9edc970

gotjoshua changed the title ~~wip Auto export snapshots~~ Auto export snapshots Jun 4, 2023

mbnuqw marked this pull request as ready for review June 5, 2023 11:57

mbnuqw merged commit 35a6223 into mbnuqw:v5 Jun 5, 2023

domportera mentioned this pull request Jun 8, 2023

Auto-export every X minutes/hours, or, SAVE/SYNC snapshots to the cloud #876

Closed

Auto export snapshots #1047

Auto export snapshots #1047

Conversation

gotjoshua commented May 23, 2023

mbnuqw commented May 24, 2023

gotjoshua commented May 24, 2023

mbnuqw commented May 25, 2023

gotjoshua commented May 25, 2023 • edited Loading

gotjoshua commented May 25, 2023 • edited Loading

mbnuqw commented May 25, 2023 • edited Loading

gotjoshua commented May 26, 2023

gotjoshua commented May 26, 2023

mbnuqw commented Jun 1, 2023

Testing:

Possibly wrong indent in md output with "Tree Friendly MD (add extra bullets for windows and tabs)":

Auto-export doesn't work with an empty path ("Path to Export to (relative to downloads)").

Firefox always asks (if configured in FF settings) where to save for auto-created snapshot.

"json" and "both" format options doesn't work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjoshua commented Jun 1, 2023

gotjoshua commented Jun 4, 2023

mbnuqw commented Jun 4, 2023 • edited Loading

mbnuqw commented Jun 4, 2023

gotjoshua commented Jun 4, 2023

gotjoshua commented Jun 4, 2023

gotjoshua commented Jun 4, 2023 • edited Loading

mbnuqw commented Jun 5, 2023

gotjoshua commented Jun 5, 2023

gotjoshua commented Jun 5, 2023

gotjoshua commented May 25, 2023 •

edited

Loading

gotjoshua commented May 25, 2023 •

edited

Loading

mbnuqw commented May 25, 2023 •

edited

Loading

mbnuqw commented Jun 4, 2023 •

edited

Loading

gotjoshua commented Jun 4, 2023 •

edited

Loading