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

saver priority update #4371

Closed
wants to merge 27 commits into from
Closed

Conversation

xmaysonnave
Copy link
Contributor

Hi Jeremy,
This PR is meant to expose at the API level a way to dynamically update the savers priority.
The use case I faced is the following:
Sometimes the only open Ipfs server I know, infura.io is very slow and as such saving a wiki instance started to be an issue. Changing dynamically the priority helps to get a chance to do a local backup.
Warmly

@pmario
Copy link
Member

pmario commented Nov 13, 2019

hmmm ... I'm not happy with this solution. It feels a bit hacky.

I would prefer a mechanism, that allows us to "chain savers". The default behavior should be as it is. The first saver, that can save does it.
But the user should be able to say: "I want to save in 2 or more ways" or "try this saver first. If it fails use this one, then that ...and so on"

@pmario
Copy link
Member

pmario commented Nov 13, 2019

My usecase is: Use WebDav saver AND local browser addOn saver

@pmario
Copy link
Member

pmario commented Nov 13, 2019

We can have a second configuration that is user facing. If it is active it contains the names of savers, that should be used _and the order. An additional flag says: 1st wins or use all

@Jermolene
Copy link
Member

Thanks @xmaysonnave is the idea that a saver plugin would call the updateSaver() method when it changes its own priority?

It would help to have a better idea of the original problem. Can you outline a scenario in more detail where a saver would want to dynamically adjust its priority?

@xmaysonnave
Copy link
Contributor Author

Hi Jeremy,

Here is the scenario I faced and you already partially understand the way I solved it.

As you know I save the wiki and attachments over Ipfs.

When I work with my Ipfs server it is usually fast and reliable however when I work with the only open Ipfs server I know (infura.io) the scenario is different.

In my TiddlyWiki instance the Ipfs with TiddlyWiki saver has the hightest priority. It means that each time I want to save, the saver is called and processed. Sometimes infura.io is not reachable and I felt locked. How to save my current wiki if the server I want to reach is not available. As priorities are hardcoded and not exposed I face an issue I wanted to solve.

(To be clear I do not pretend here that my suggestion is the best possible solution as I'm in my TiddlyWiki learning process).

To unlock this situation I found this way. Modifying dynamically the Ipfs plugin priority gives a chance to use the default saver (download file saver).

Do not hesitate to comment.

Warmly.

@bimlas
Copy link
Contributor

bimlas commented Dec 20, 2019

I did almost the same, but in this case you can deicide which saver to use:

https://bimlas.gitlab.io/demo/tw5/preferred-saver.html

Combined with your solution, this would mean choosing from a dropdown menu which saver gets the highest priority.

@xmaysonnave
Copy link
Contributor Author

@bimlas I like your idea to display a dropdown with the available savers.
It means that in a single place we can choose which one we want.
However I see two solutions :

  • we need to reorganize the savers priority
  • we don't touch the default savers priority but a preferred one is always used (the default is the highest when empty or the choosen one when reloaded), probably easier to implement....
    Nice...

@bimlas
Copy link
Contributor

bimlas commented Dec 20, 2019

@pmario

Chaining seems like a good solution because it allows the user to determine their own order of importance. It can be easily accomplished if e.g. they would be listed in a draggable list where you can move them up and down. Each list item would have a radio button that would make the selected saver its highest priority, so we could move the selected saver to the "top of the list" temporarily. That way, I think each solution could come together.

To have an "AND" connection, there could be a list of lists.

<ul>
    <li>
        <input type="radio"/> Pmario group
        <ul>
            <li>WebDAV saver</li>
            <li>Local saver</li>
        </ul>
    </li>
    <li>
        <input type="radio" /> Git group
        <ul>
            <li>GitHub saver</li>
            <li>GitLab saver</li>
        </ul>
    </li>
    <li>
        <input type="radio" /> Group with single elem
        <ul>
            <li>AWS saver</li>
        </ul>
    </li>
</ul>

@pmario
Copy link
Member

pmario commented Dec 20, 2019

@bimlas ... I would be OK with the "group lists". So the 1st level would make it active and the second level lists the savers, that should be used.

I like this idea.

@Jermolene
Copy link
Member

Perhaps the best way to approach this might be less subtle to (a) add a new filter that exposes the names of the available savers and (b) to add a parameter to the download/save messages allowing the saver to be explicitly specified. Then in your use case one could add a secondary save button to force a save to a different saver if the primary saver has failed.

@xmaysonnave
Copy link
Contributor Author

@Jermolene Thanks for your comments. It's quite funny as your mail arrived precisely when I was working on a solution...

@pmario I implemented a list a checkbox to accomodate users who need to call multiple savers at once.

Here is a screenshot:

PreferredSaver

I think I use what you described in point a (macro and filter operator) :

Tiddler:
https://github.com/xmaysonnave/tiddlywiki-ipfs/blob/master/tiddlers/plugin/core/ui/ControlPanel/Saving/General.tid

Macro:
https://github.com/xmaysonnave/tiddlywiki-ipfs/blob/master/src/macro/ipfs-info-saver.js

FilterOperator:
https://github.com/xmaysonnave/tiddlywiki-ipfs/blob/master/src/operator/ipfs-savers.js

SaverHandler:
https://github.com/xmaysonnave/tiddlywiki-ipfs/blob/master/src/core/saver-handler.js

The important point here is that savers like mine could be async by nature.
As a consequence the SaverHandler needs to await each savers to get a chance to fallback.

In summary:

  • @bimlas Only canSave savers are loaded and instantiated
  • @pmario Multiple savers can be processed
  • The $tw.saverHandler.saveWiki is async
  • Users have a chance to select the appropriate savers if needed without any priority considerations

Here is an ipfs link with a functional tiddly:
https://gateway.ipfs.io/ipfs/bafybeigfoej46swyjloybs2vmj3mpb7meqir7kc47gdwpq365pessolkbq

Thanks

@pmario
Copy link
Member

pmario commented Apr 16, 2020

I did just download your master branch, and it doesn't contain the changes. ... So please create a new branch, so we can test the stuff.

@xmaysonnave
Copy link
Contributor Author

You're wrong. The changes are there...

@pmario
Copy link
Member

pmario commented Apr 16, 2020

Yea, but they are not part of the PR anymore.

@pmario
Copy link
Member

pmario commented Apr 16, 2020

I found the stuff ... thx

@xmaysonnave
Copy link
Contributor Author

It's probably better to close this PR and open a feature request.

@xmaysonnave
Copy link
Contributor Author

I would prefer to push a PR if we reach a consensus. Thanks

@pmario
Copy link
Member

pmario commented Apr 16, 2020

I'm very interested in the possibility to enable 2 savers at the same time. ... And I think it should be part of the core.

The boot.js is minified then TW do not entirely ship readable and
debuggable code...

You are right. sjcl.js is minified, which is the 3rd party encryption library, that TW uses. Most 3rd party libs are included in minified form, since this saves space.

Just a reminder I don't need this feature, you need it. The drop down is
enough for me then no async/await...

That's good to know. TW is callback oriented. ... I'll have to have a closer look. We may be able to adjust it accordingly.

@xmaysonnave
Copy link
Contributor Author

I've created a specific branch to ease our follow up.
https://github.com/xmaysonnave/tiddlywiki-ipfs/tree/savers
Thanks

@xmaysonnave
Copy link
Contributor Author

Forget my previous comment and sorry for the confusion.
I setup two branches.

A multi preferred savers implemented as a list of checkboxes as previously described.
https://github.com/xmaysonnave/tiddlywiki-ipfs/tree/multi-preferred-savers

and a single preferred saver as a dropown list.
https://github.com/xmaysonnave/tiddlywiki-ipfs/tree/single-preferred-saver

Remark: It's not quite clear to me how to combine the async nature of my saver and the callback. In fact I was wrong in one of my comment. Even a single preferred saver needs to await otherwise no chance to fallback in case the async saver return false.

Thanks

@pmario
Copy link
Member

pmario commented Apr 17, 2020

Remark: It's not quite clear to me how to combine the async nature of my saver and the callback. In fact I was wrong in one of my comment. Even a single preferred saver needs to await otherwise no chance to fallback in case the async saver return false.

Thanks for the clarification. ... I think to keep the code simple and easy to understand we should just use async functions as you did. ... Package everything into a plugin, without the need to add babble as a dev-dependency. (I'm talking about TW savers only. Your IPFS setup is OK)

TW core uses ES5 features only. So no let or const and all the other stuff that was introduced with ES6(aka ES2015).

@pmario pmario mentioned this pull request Apr 30, 2020
@Jermolene
Copy link
Member

Hi @xmaysonnave as well as the saver-priority changes, I'm also seeing a bunch of changes concerned with adding compression to the single file edition. Is there some dependency between these changes? Otherwise they should be separate PRs.

@xmaysonnave
Copy link
Contributor Author

Hi Jeremy, I'm preparing a new PR with data compression. It's not related to the saver priority though. I'll probably push it tomorrow.

@Jermolene
Copy link
Member

Hi @xmaysonnave there are 18 changed files in this PR, and they include the changes for adding compression. Please can you remove those changes from this PR so that I can review it?

@xmaysonnave
Copy link
Contributor Author

Hi Jeremy. I see what you mean. I didn't pushed any updated pr about the saver priority however I merged your latest commit to my forked repo to be synced with my compression branch. The consequence is confusing and unexpected. Let me clean up and I come back to you. Thanks for your feedback.

@Jermolene
Copy link
Member

Thanks @xmaysonnave!

@pmario
Copy link
Member

pmario commented Jun 14, 2020

I think the compression "part" needs to be a plugin. TW already has a jsZip plugin, that seems to have a similar functionality. ... compression would / will also add a 3rd party dependency to the core, which should be avoided.

@pmario
Copy link
Member

pmario commented Jun 14, 2020

It seems, that compression and decompression needs some low level changes to boot.js. I think, the only way to include 3rd party mechanisms here is to add some "th-compress, th-decompress" hooks at the right places. So plugin authors are able to use them in a defined way.

@xmaysonnave
Copy link
Contributor Author

Hi Jeremy,
I'm going to close this PR and open a new one as I don't want to introduce more confusion.
Thanks

@xmaysonnave xmaysonnave mentioned this pull request Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants