anyproto / anytype-ts

Official Anytype client for MacOS, Linux, and Windows
https://anytype.io
Other
3.73k stars 220 forks source link

Last opened object homepage setting with window context #695

Closed mikailcf closed 2 months ago

mikailcf commented 2 months ago

Description

What

This PR makes it so that the Last opened object homepage setting now considers the context of different windows (i.e. different windows have different last opened objects and each is loaded accordingly when the Last opened object home page setting is selected).

Also when closing a window its object is removed from the local storage Last opened context and when closing the app all objects, besides the last opened for by the first window, are removed from the local storage.

Why

The reasoning here is based on my usage and is open for discussion

Practical reason

Currently when users have the Last opened object homepage selected and have multiple windows opened, any time any of those windows are refreshed it then loads the last opened object period (which in many cases was opened in another window).

One example in my workflow is: I have a window with a set of active notes on different topics and frequently either create a new note in the set and open it in a new window or open an active note in a new window. The window with the set of notes should always only display that object, but opening new notes on new pages overrides the Lsat opened object and after having the computer go to sleep and wake it up again, now all windows show the same object (which was the created/opened note).

Reasonable... reason

It seems reasonable to me that users with the Last opened object homepage selected wouldn't want all windows to load the same object after a mass refresh (such as the computer waking up from sleep) 😜 .

What type of PR is this? (check all applicable)

Related Tickets & Documents

Added tests?

Added to documentation?

github-actions[bot] commented 2 months ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

mikailcf commented 2 months ago

I have read the CLA Document and I hereby sign the CLA

mikailcf commented 2 months ago

recheck

ra3orblade commented 2 months ago

I do not like that you have changed Storage.set, it should not contain this logic. Storage.set should not know any context about the key that is being set and you have incorporated this logic inside Storage.set.

All helping method that store objects should be separate, like Storage.setToggle, Storage.setScroll and etc.

Second thing - check formatting and linting please.

mikailcf commented 2 months ago

Do you mean the change I made when calling Storage.set?

The responsibility of knowing the context is in the caller (ObjectOpen function) in this case. The implementation of Storage.set is unchanged.


I fixed the linting. If you have any other style requests I'd ask you to comment in the specific lines, please.

ra3orblade commented 2 months ago

Sorry, I've mixed the method when I was writing and was looking at unsetPath.

I mean the whole logic can work the same way it works in other helpers.

setLastOpened (windowId: string, param: any) {
    const obj = this.get('lastOpened') || {};
    obj[windowId] = Object.assign(obj[windowId] || {}, param);
    this.set('lastOpened', obj, true);
};

getLastOpened (windowId: string) {
    const obj = this.get('lastOpened') || {};
    return obj[windowId];
};
mikailcf commented 2 months ago

Oh I see now, good idea. I've committed a new implementation following that.

mikailcf commented 2 months ago

@ra3orblade sorry about those approvals. I've mentioned that I had an open PR in an open source project to some coworkers and they got ahead of themselves and thought they could approve the changes. I've already dismissed their reviews.


About your requests: I'll push the necessary changes in a few hours.

fuksman commented 2 months ago

@any contributor @mikailcf code