add abortable before events for sorting, filtering, pagination and maybe grouping

[der-david]

I'm submitting a Feature request

Motivation / Use Case

When using a backend service (e.g. OData) and modified (edited, added, removed) data has not been sent to the server, yet, changes to filters, sorting, pagination and possibly grouping can cause loss of the modified data. I would like to be able to intercept and prevent these option changes to warn the user of the potential loss of unsaved changes.

Expected Behavior

Additional onBefore... (abortable) events or optional functions should be available/providable that allow for interception (and abortion) of those option changes.

Other Information

I'm willing to help implement these features but I'm open for other ideas, feedback and possible workarounds.

Best regards
David

[ghiscoding]

hmm I'm not sure to fully understand the use case, would it be possible to provide some kind of code/data sample?
You want onBefore for which event(s)?
All the backend interfaces are in this folder common/src/interfaces/ and both OData/GraphQL follow these interfaces.

I'm open for contributions if you can provide a bit more demo/code to explain the use case that'd be great too

[der-david]

Sure, I'll try, but setting up a codebox, fiddler etc. is kinda cumbersome with an OData service and all that. So I'll try it with some insights and references from my tests and findings.

Image you have an OData-bound grid with pagination, sorting and filtering.
It is configured to have (some) editable cells (without a modal window) and button functionality is added outside of the grid to send the modified values to the backend to persist them.
However, that does not happen automatically but the user has to explicitly Save the changes (the cells will be highlighted until the changes have been saved).
So in essence we have a combination of Example 09 and Example 11.

Due to the in-place editing feature, pagination, resorting, filtering etc. is currently not prevented, although there might be unsaved changes (which are stored in an edit queue). So whenever the user navigates, filters etc., the changes disappear from the UI (still in the edit queue) without giving the user the opportunity to persist those changes beforehand.

E.g. if I use the default pagination service and component, clicking the "Next Page" button is not directly interceptable or the pagination changes would have to be reverted at a later point (__pageNumber is increased immediately).

slickgrid-universal/packages/common/src/services/pagination.service.ts

Lines 189 to 195 in 20564a3

	goToNextPage(event?: any): Promise<boolean | ServicePagination> {
	if (this._pageNumber < this._pageCount) {
	this._pageNumber++;
	return this.processOnPageChanged(this._pageNumber, event);
	}
	return new Promise(resolve => resolve(false));
	}

Clicking on a column header for sorting is handled directly within slick.grid.js, so a feature implementation would probably affect both repos.
https://github.com/6pac/SlickGrid/blob/3991679be77841d1abe86771060b67a1a38d28f2/slick.grid.js#L1328

    function setupColumnSort() {
      $headers.click(function (e) {
        if (columnResizeDragging) return;
        // temporary workaround for a bug in jQuery 1.7.1 (http://bugs.jquery.com/ticket/11328)
        e.metaKey = e.metaKey || e.ctrlKey;

        if ($(e.target).hasClass("slick-resizable-handle")) {
          return;
        }

        var $col = $(e.target).closest(".slick-header-column");
        if (!$col.length) {
          return;
        }

        var column = $col.data("column");
        if (column.sortable) {
          if (!getEditorLock().commitCurrentEdit()) {
            return;
          }
...

While it is probably possible to intercept (and later revert changes in pagination, sorting, filtering etc.) based on the callstack using a custom EditorLock that overrides the Slick.GlobalEditorLock and its isActive method, that seems quite specific and doesn't really feel right.
So I'd like to have maybe beforePaginationChange, beforeSortingChange, beforeFilterChange and beforeGroupingChange (you get it) events, that I can hook to with like a custom validator, that can possibly abort the change or save the modifications before applying the change.
Optional methods within the grid options would be another possible implementation.

I know this is a lot to ask for and I'm sure it's just not already baked in by design because of SlickGrid(-universal)'s kind of Frankenstein's heritage (no offense whatsoever, I love it so far 😊).

Until we have this feature, I will probably build a temporary solution to disable the relevant UI components while the editQueue is not empty. I'd be glad to contribute, but I would like to hear your thoughts and possible solutions first, since you're waaaaay more familiar with the overall architecture and quirks.

Thanks for your time, have a nice weekend and stay safe
David

[ghiscoding]

I know this is a lot to ask for and I'm sure it's just not already baked in by design because of SlickGrid(-universal)'s kind of Frankenstein's heritage (no offense whatsoever, I love it so far 😊).

I created the lib from scratch over the years, all of it (my own heritage), it's a wrapper on top SlickGrid but most of the code come from Aurelia-Slickgrid/Angular-Slickgrid, I know it's a bit confusing but it's mainly because I had 2 libs already (Angular-Slickgrid, Aurelia-Slickgrid) and I had to create a 3rd grid lib for another project (in Salesforce which uses ES6 syntax), so I made this Slickgrid-Universal as a common lib that could eventually be used by all other libs and I do bug fixes in 1 place instead of replicating in 3 places (now Aurelia-Slickgrid does use Slickgrid-Universal since last Christmas so it's 1 less to maintain for bugs, Aurelia-Slickgrid is down from 11,000 lines of code to just over 1,000 lines of code now because the main code is now coming from Slickgrid-Universal), I know it might confuse people because now I have a wrapper (aurelia-slickgrid/slickgrid-universal) on top of a wrapper (slickgrid) but hey I didn't want to rewrite SlickGrid itself so yeah there's a lot to wrap your head around hehe

On adding the onBeforeX, I'd be ok with this but you'll find soon enough that the backend code is a bit spreaded in the code, some parts are in Filter Service, Sorting Service, Grid State Service and vanilla-bundle (or aurelia-slickgrid if you use that lib).

The main thing to note is that I use SlickGrid DataView in every cases, even with backend services and in that case I just replace the DataView dataset (with dataview.setItems()) whenever I change page or change filter. The reason I use DataView in every cases is that my code is the same for the entire project and that I can also use features like Grouping (however it will group only with current page backend data). The things to note though with that approach is that when using backend service, if we have let say 20 items per page and we add an item to the page it will in fact show as 21 items because we push to the DataView and the DataView is equal to a page, it will remain 21 items until we change page or we refresh the grid (because that would call the backend with a query) and some user might not like that approach. On another project I've worked on, they wanted to push the item to the end, so what I've done was to use the pagination service to go to last page then insert the item in the DataView so that it shows as the last item in the grid. Basically the DataView is not directly connected to the backend, there are some events that says "hey DataView, I got new data please refresh re-render the grid with new data" but yes there's currently no mechanism for backend failure (which what you're trying to deal with in this issue I assume).

We used the GraphQL Service in my previous project and we did some CRUD, but never really dealt with auto-reverting if something goes wrong, we just display an error and people tend to refresh in general so that was ok (I guess another thing you could do is refresh the grid by calling backend if you encounter an error). When doing edits, if the backend fails, we were just reverting the value in the cell (inline editing) and if it was successful then I always ask for the backend to return the row(s) that was updated (if the backend does any kind of calculations that affects other field then it would be reflected right away because I use the backend response to update the entire row item object), that's usually enough for our usage.

I'm totally open for contributions, so if you got something working then please go ahead and create a PR. I can provide guidance for sure.

Last question, are you using Slickgrid-Universal directly or you're using it from Aurelia-Slickgrid? I'm just curious, because so far I got no one using Slickgrid-Universal directly (I think), except my team for our Salesforce implementation.

[ghiscoding]

I think I start to finally understand what you want, it helps to brainstorm in the shower lol.. so if I understand correctly, you want a way to cancel/revert any changes that is page change, edits and whatever else and you want something like calling onBeforePagination returning false would cancel the pagination increase/decrease and whatever else. I never thought of coding it that way, it would probably require a lot of code changes in a few areas and I'm actually not sure how to code that approach. I assume that something like an onSort event

slickgrid-universal/packages/common/src/services/sort.service.ts

Lines 69 to 70 in 20564a3

	const onSortHandler = grid.onSort;
	(this._eventHandler as SlickEventHandler<GetSlickEventType<typeof onSortHandler>>).subscribe(onSortHandler, this.onBackendSortChanged.bind(this));

would require to add an onBeforeSort in SlickGrid (core) lib and if that returns false then cancel the onSort, which I guess is the exact same approach as the onBeforeEditCell in SlickGrid, so that would require some extra events in SlickGrid and then use them in here.

[der-david]

I created the lib from scratch over the years, all of it (my own heritage)...

Yeah, I know that and I really like your modularization concept and carving out redundant code is always a good idea 👍. But to be honest I wasn't even aware that the original SlickGrid is still used under the hood 😅. I was really surprised, when I saw slick.grid.js and jQuery in debugger a few days ago. I somehow just assumed you had completely rewritten all of SlickGrid given the extreme effort you put in and the huge codebase provided. But of course there is no real point in rewriting all of that and trying to keep up with the fixes and developments over at 6pac/SlickGrid. Hence my Frankenstein's reference, all love.

Last question, are you using Slickgrid-Universal directly or you're using it from Aurelia-Slickgrid? I'm just curious, because so far I got no one using Slickgrid-Universal directly (I think), except my team for our Salesforce implementation.

I use slickgrid-universal, since we already have a frontend framework in use but it lacks components for spreadsheet-like features.
I have set up a scenario in which SlickGrid provides a dynamic interface to arbitrary (user configured) OData services from various sources based on the $metadata.

I assume that something like an onSort event would require to add an onBeforeSort in SlickGrid (core) lib and if that returns false then cancel the onSort, which I guess is the exact same approach as the onBeforeEditCell in SlickGrid, so that would require some extra events in SlickGrid and then use them in here.

Exactly! I'll have a look to find out where the modifications could be placed and try different solutions. Of course, changes to slickgrid-universal would require those to SlickGrid core to be accepted first, but I guess @6pac would be cool with it.

Best regards
David

[ghiscoding]

I'm a contributor on the 6pac/SlickGrid because I made lot of changes, fixes, features, plugins, ... and yes the point that there's no need to rewrite something that already exists, so I've pushed lot of fixes on the SlickGrid fork and that benefits both lib and both user base.

You should probably start with the onBeforeSorting, I assume that would be the easiest to start with and again I assume it would follow the same concept as onBeforeEditCell, something like this should work, basically what I've done was to wrap trigger(self.onSort... inside an extra if (trigger(self.onBeforeSorting... and both will provide the same args

if (!options.multiColumnSort) {
  var multiSortEventArgs = {
    multiColumnSort: false,
    columnId: (sortColumns.length > 0 ? column.id : null),
    sortCol: (sortColumns.length > 0 ? column : null),
    sortAsc: (sortColumns.length > 0 ? sortColumns[0].sortAsc : true)
  };
  if (trigger(self.onBeforeSorting, multiSortEventArgs) === true) {
    trigger(self.onSort, multiSortEventArgs, e);
  }
} else {
  var singleSortEventArgs = {
    multiColumnSort: true,
    sortCols: $.map(sortColumns, function(col) {
      return {columnId: columns[getColumnIndex(col.columnId)].id, sortCol: columns[getColumnIndex(col.columnId)], sortAsc: col.sortAsc };
    })
  };
  if (trigger(self.onBeforeSorting, multiSortEventArgs) === true) {
    trigger(self.onSort, singleSortEventArgs, e);
  }
}

The only problem I see with that though is that it requires the user to always return true on the onBeforeSorting event and that is a breaking change (most people won't use the onBeforeSorting). It would have to be inverse to not be a breaking change (just like onBeforeEditCell only compares against === false), it might be counter-intuitive in the onBeforeSorting but we really have to return false (there's a problem) or true (all is good)

or actually wait, we just have to do it exactly like onBeforeEditCell and just return if we see a false, I guess that'd be ok since there's no other code after the event.

if (!options.multiColumnSort) {
  var multiSortEventArgs = {
    multiColumnSort: false,
    columnId: (sortColumns.length > 0 ? column.id : null),
    sortCol: (sortColumns.length > 0 ? column : null),
    sortAsc: (sortColumns.length > 0 ? sortColumns[0].sortAsc : true)
  };
  if (trigger(self.onBeforeSorting, multiSortEventArgs) === false) {
    return;
  }
  trigger(self.onSort, multiSortEventArgs, e);
} else {
  var singleSortEventArgs = {
    multiColumnSort: true,
    sortCols: $.map(sortColumns, function(col) {
      return {columnId: columns[getColumnIndex(col.columnId)].id, sortCol: columns[getColumnIndex(col.columnId)], sortAsc: col.sortAsc };
    })
  };
  if (trigger(self.onBeforeSorting, multiSortEventArgs) === false) {
    return;
  }
  trigger(self.onSort, singleSortEventArgs, e);
}

or wait again, I was actually going too complicate for no reason, the following would also work since undefined is not false

  if (trigger(self.onBeforeSorting, multiSortEventArgs) !== false) {
    trigger(self.onSort, multiSortEventArgs, e);
  }

[der-david]

Of course I want the changes to be non breaking as optional features should be.
Since sortColumns is not a local handler variable but modified directly, I opted to make a temporary backup that can be reverted to, in case trigger(self.onBeforeSorting, ...) returns false.
der-david/SlickGrid@5479cba
This way the new onBeforeSort handler could even compare the old and new sorting and decide whether to continue based on that. Plus, setSortColumns has not been called and the UI has not been changed, yet.
Do you have any objections?

[6pac]

All good with me. The ony thing I'd say is to mention the tension between say OnUpdate versus BeforeUpdate and AfterUpdate. I prefer the latter because it's more specific. I don't really like OnBeforeUpdate as I think it's unnecessarily mixing the two paradigms. However we have legacy named events to consider, which may restrict the possibilities, unless we deprecate but retain existing events and encourage a second newly named version.

[ghiscoding]

@6pac you would prefer to see what in the case of Sorting then? I was ok with the onBeforeSort mainly because I use the onBeforeEditCell all the time but I'm curious to see what would be your preference in terms of Sorting, the goal that @der-david is aiming for is basically to revert to the previous state (or simply cancel the action) in case the backend fails (or to go ahead when backend is all good). Personally I find onBefore better representing the (before the fact) vs the onAfter (after the fact), I find it easier to cancel an action vs rolling back an action.

[6pac]

I'm only referring to the naming. OnSort for example is ambiguous becuase it could be before or after. Frequently there is a multi step process, and the user may need to hook in at various stages.
For sorting I'd prefer BeforeSort and AfterSort. I think the 'before' and 'after' replace the 'on' as the preposition, so OnBeforeSort is redundant. But perhaps I'm just pedantic ;-)