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

Persistent facets using localStorage and superclass/subclasses for facets objects #2685

Closed

Conversation

antoine2711
Copy link
Member

@antoine2711 antoine2711 commented Jun 8, 2020

This is a basic and minimalistic implementation to save facets. It is VERY simple, both in code and user implementation.

A new icon in form of a disk image (yes, it's small), when pressed, will save the facet for this project in the browser data, overwriting the configuration of the same facet if it already existed, otherwise, creating a new one.

If a facet is deleted, so is the data in localStorage for that facet. Nothing happens if a facet is minimized.

image

This issue is a prototype to start building functionality for facets. By creating a super class for facets, GenericFacet, it enables adding functionality to all facet from one JS object.

Please note that this is only implemented for ListFacet. If it get support from the community, I will code the rest. Also, a three state (not saved, saved, dirty) could be implemented. If the icons were provided, I would code the rest (image = main/webapp/modules/core/images/save-facet-map.png).

Yes, many other functionalities could be added before committing to master, but I believe this is the first needed step, at least the most easy in term of coding/reliability/functionality.

It is vaguely related to I #560.

First implementation, partial
Functional implementation of saving facets
@antoine2711 antoine2711 marked this pull request as draft June 8, 2020 05:05
@antoine2711
Copy link
Member Author

antoine2711 commented Jun 8, 2020

@thadguidry & @wetneb: what I was referring to in my email. It's a simple implementation. But it is bug free and functional at 100 %. Very simple yet effective.

Regards,
Antoine

Fix a bug when opening new projects for the first time.
@wetneb
Copy link
Sponsor Member

wetneb commented Jun 9, 2020

I am curious about the motivation behind the design. Personally, I would prefer not to have to explicitly save the facets, let alone one by one: I would just like that when I reopen a project, the facets I used when I last worked on it are restored. Could you describe a bit more the workflows you have in mind with this prototype - in which situation would you want to consciously save one facet for later?

@antoine2711
Copy link
Member Author

antoine2711 commented Jun 9, 2020

I am curious about the motivation behind the design. Personally, I would prefer not to have to explicitly save the facets, let alone one by one: I would just like that when I reopen a project, the facets I used when I last worked on it are restored. Could you describe a bit more the workflows you have in mind with this prototype - in which situation would you want to consciously save one facet for later?

Hi @wetneb.

  1. I don't really code Java, so I tend to only think/operate on the client side. I don't fear Java, but I will try to avoid it.
  2. All my features suggestions since a few months are about removing pain. Not thinking the best feature, but the easiest/simplest way to remove limitations.

That being said, when I work in OR, I have 2 kinds of facets. The first, the obvious and simple one-click, and complex/built/custom ones. The latter are the one I wanted to save & keep.

The solution you propose, if I understand correctly, is to always save facets. This way would be better. But, what do we do with a second tab?

Imagine this scenario:

  1. Open a new project
  2. Reopen the same project in a second tab
  3. Create 3 facets in tab Gridworks should allow programmatic removal of row #1
  4. Create 3 facets in tab Undo History bug #2
  5. Reload tab Gridworks should allow programmatic removal of row #1, and, with your solution: 6 facets. my solution, only saved facets in both.

Hard to say what's the best. Managing multi-tab is interesting. One way is that a project could « save » many sets of facets. If you choose the same set on 2 tabs, every change in a tab could be automatically applied to the other tabs with the same loaded set… This is nice, but would probably need saving on the server side (Java) for the sets.

Is it more clear why I choose saving on a per facet?

Regards,
Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented Jun 9, 2020

Multi-tab scenarios are interesting, but for now OpenRefine is not really designed for these I would say: at the moment the second tab is not going to refresh if you make changes in the first one, and so on.

I would not say that in my scenario you would end up with 6 facets: you would end up with whichever tab's facets were saved last (so in your scenario, tab 2).

@antoine2711
Copy link
Member Author

antoine2711 commented Jun 9, 2020

Multi-tab scenarios are interesting, but for now OpenRefine is not really designed for these I would say: at the moment the second tab is not going to refresh if you make changes in the first one, and so on.

I would not say that in my scenario you would end up with 6 facets: you would end up with whichever tab's facets were saved last (so in your scenario, tab 2).

@wetneb: in this code, when I store a facet, I store it with a unique id. That's so that if I change it, and resave it, the same facet gets kept and it doesn't do a duplicate. In your scenario of 2 tabs, it would be a « last save war », and the complex facets on one side would been annihilated. That would be « painful » for the user.

In the way I write it, if you create a « flagged » facet in each tab, and save them, a reload of the page will show them both. If you make a selection in one of them, that selection is also saved. Saving, in this code, is really taking a snapshot of a facet at a moment, will all it's data.

I really want to avoid the « last save war » scenario for preserving facets. My way give assurance that the facet you deem important are kept.

What do you think of all that? Should we avoid a « last save war » scenario like I want?

Regards,
Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented Jun 10, 2020

I would reject multi-tab scenarios as outside the scope of the tool as it currently stands. I do not think this use case is well-supported enough to motivate design choices like this one. I totally think it would be very useful to think about how multi-tab workflows could be supported but facets persistence is not where I would start: for instance, we would first need to make sure that when an operation is applied in one tab, any other tab also sees it. I do not think it makes much sense to design facet persistence scenarios without that, since facets depend on the operations applied so far, so it would not make much sense to restore in one tab a facet that was created in another tab without making sure that the grid is up to date in the first place…

I am really keen to think more about multi-tab scenarios but I really think that sort of discussion needs to be built on top of concrete workflows, so that we understand which use case we are targetting. If you want to work on a project with multiple tabs open, what are the roles of each of these tabs? For me, I can think of use cases where each tab could have a different set of facets, such that each tab lists rows with some sort of data validation problem. As I clean the data, these tabs would update, and I would be able to keep an eye on the number of rows with each sort of problem. I could work in each tab to fix these specific problems without having to change the facets every time. That being said, it is not clear to me why you would want to switch between those tabs in a random order, rather than cleaning up all problems of type 1 first, changing the facets to surface problems of type 2, cleaning it up, and so on (which is what I would do with the current one-tab setup). This does not have any overhead since you only set up the facets for each problem once.

Perhaps the most compelling use case could be the reconciliation of two different columns: in each tab you could select rows with cells to match in a different column, and work on these independently - but I think that really comes down to multi-user support (you really want multiple users to be able to work on these tabs at the same time, otherwise the benefit of a single user being able to switch tabs when they get bored with their current task is quite low I would say).

@antoine2711
Copy link
Member Author

antoine2711 commented Jun 10, 2020

I would reject multi-tab scenarios as outside the scope of the tool as it currently stands. I do not think this use case is well-supported enough to motivate design choices like this one. I totally think it would be very useful to think about how multi-tab workflows could be supported but facets persistence is not where I would start: for instance, we would first need to make sure that when an operation is applied in one tab, any other tab also sees it.

Hé, hé, hé… the multi-tabs scenario opens a lot of interesting questions. I myself have « stages of cleaning up », but I'm not sure how that would apply to other user. Your scenarios are interesting and should go in a «multitab regional roadmap… »
;-)

I'm only mentioning multi-tabs because it must not break the implementation by deleting all the stored facets if closed last.

For sure, there is the clean data/refine data/push data thru multiple axes & operations, and all those could require different sets of facets. But I really think this is beyond the scope of this draft pr/prototype.

I mentioned multiple tabs, because I use them sometime. Changing updating the facets for the user would seam bad, a work around would be to ask if the user wants it, but still, it's agressive, and doing things the user hasn't ask.

That why, this little tool, which makes a facet « stick », is simple and useful.

Saving sets of facets in the project/java would be better, and would kill this feature, but meanwhile, this is better than nothing.

Would be worth doing in itself, wouldn't it, @wetneb?

Regards,
Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented Jun 10, 2020

That why, this little tool, which makes a facet « stick », is simple and useful.

There are some areas where it feels safe to do small changes that address clear usability bottlenecks (such as making the left panel collapsible, without cleaning up the whole UI resizing logic), but for this I would prefer to get it right from the start.

This is an area where there are many design questions open, and introducing a small fix like this could restrict our freedom in redesigning the entire system down the line.

@antoine2711
Copy link
Member Author

antoine2711 commented Jun 10, 2020

@wetneb:

You haven't talked about the ES6 implementation of superclass/subclasses illustrated in this prototype. What do you think? I would thinks ES5 is better than this, much less code change, but older way also, less standard (https://medium.com/beginners-guide-to-mobile-web-development/super-and-extends-in-javascript-es6-understanding-the-tough-parts-6120372d3420).

Also, we could make this feature « on an activation base thru prefs », a bit like flags in Chrome. This lowers expectation, while enabling the functionality for the most daring… ;-) Anyway, I understand some of your concern, and if I had more time, I would address them.

Regards,
Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented Jun 10, 2020

Yes using classes like this seems appropriate - I would need to look into the differences between ES5 and ES6 here.

@antoine2711
Copy link
Member Author

antoine2711 commented Jun 11, 2020

Yes using classes like this seems appropriate - I would need to look into the differences between ES5 and ES6 here.

@wetneb the link I provided is pretty clear, here's the summary.

ES5

function Gorilla(name, weight) {
    Animal.call(this, name, weight);
}

Gorilla.prototype = Object.create(Animal.prototype);
Gorilla.prototype.constructor = Gorilla;

ES6

class Gorilla extends Animal {
    constructor(name, weight) {
        super(name, weight);
    }
}

With ES5, there will be far less rewriting. More moving code around.

Regards,
Antoine

@antoine2711
Copy link
Member Author

antoine2711 commented Jun 15, 2020

@abubelinha, here's the version compiled by me. It's a v3.4b from a few days.

http://www.beaubien.info/OpenRefine/persistent-facets-on-openrefine/

image
What new here is the « disk » icon. Once you press it, it saves the facet in the browser. If you modify the facet, it will not update the saved state, unless you reclick on the « disk ».

Note that if you close the facet, it deletes it from the browser’s data. Play with it, and tell me how you like it.

Regards,
Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented Jan 18, 2022

I am going through the open pull requests and would propose to close this one, given that I think we should be very careful about starting to rely on browser-held state. I would rather have a solution which persists facets in the backend and without the need for a specific user interaction, which would feel more natural to me. Feel free to reopen if you disagree.

@wetneb wetneb closed this Jan 18, 2022
@antoine2711
Copy link
Member Author

I would rather have a solution which persists facets in the backend and without the need for a specific user interaction, which would feel more natural to me.

This Issue was not really about using localStorage but rather just saving facets. LocalStorage was juste a cheap way to get that.

If you think this should be done by API to the server, I will comply.

@wetneb : Should I create a new issue?

Regards, Antoine

@wetneb
Copy link
Sponsor Member

wetneb commented Jan 18, 2022

it is totally worth having an issue about this indeed!

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.

None yet

2 participants