VISAB-ORG / VISAB

VISAB is a standalone utility to visualize artificial intelligence agent behavior in games.
1 stars 0 forks source link

E make sessionoverview mvvm compliant #99

Closed leRoe93 closed 3 years ago

leRoe93 commented 3 years ago

Current implementation of the session overview as a try to make it more MVVM compliant.

The grid is now created in the view and is based on the session status information retreived from within the viewmodel.

An updateLoop is running in the view to constantly update the session gtrid based on session statuses.

Furthermore the button to clear inactive sessions now has logic, that actually works.

As already indicated in the last commit, currently the update loop is interrupted by an on stage close Consumer that is added from the view, because the view has got the update thread. For this I placed a getter for the Scope so that the view can access the list of stage closing handlers -> I know this should be in conflict with MVVM.

As of now I am not completely sure how to do this elegantly with property binding or communication, maybe we can discuss on this :)

mfroeh commented 3 years ago

Looks like a very solid approach for my understanding.

As already indicated in the last commit, currently the update loop is interrupted by an on stage close Consumer that is added from the view, because the view has got the update thread. For this I placed a getter for the Scope so that the view can access the list of stage closing handlers -> I know this should be in conflict with MVVM.

View knowing the ViewModel is fine. The viewModel itself even is a field in the view right. In real MVVM, the ViewModel is not allowed to know the view. They should only communicate through bindings. MvvmFx extends this further with the publish-subscribe pattern between View and ViewModel.

The solution you currently have does not violate MVVM, but I agree that its kinda ugly 😄 Sadly, getting the stage that the current view is part of, is not quite that simple. You probably already found out that scopes can only be injected into types implementing ViewModel (I don't know why they decided against being able to inject it into views aswell, would also be a nice way to communicate between View and ViewModel).

leRoe93 commented 3 years ago

Adjusted the getter to another name and added the missing updates for the session count labels. The list within the view model was indeed useless, because the sessions will always get retrieved from the WebApi anyway. I think now it should be ok.

mfroeh commented 3 years ago

In terms of mvvm looks good to me! I just took a look in the classes themselves and saw that there was a lot of leftovers from stuff I did, which Im guessing can all be removed right? I have one last suggestion for the sorting part.

private List<SessionStatus> sortSessionStatuses(List<SessionStatus> sessionStatusList) {
    Collections.sort(sessionStatusList, byLastRequest);
    return sessionStatusList;
}

Comparator<SessionStatus> byLastRequest = new Comparator<SessionStatus>() {
    public int compare(SessionStatus o1, SessionStatus o2) {
        return (-1) * o1.getLastRequest().compareTo(o2.getLastRequest());
    }
};

Comparator<T> is a functional interface, which means that we can pass a lambda expression for it. By abusing this we can reduce the code above, to the more readable

private List<SessionStatus> sortSessionStatuses(List<SessionStatus> sessionStatusList) {
    // Order by time of last request descending
    Collections.sort(sessionStatusList, (o1, o2) -> (-1) * o1.getLastRequest().compareTo(o2.getLastRequest()));
    return sessionStatusList;
}
leRoe93 commented 3 years ago

I just took a look in the classes themselves and saw that there was a lot of leftovers from stuff I did, which Im guessing can all be removed right?

Removed the FileSavedSubscriber inner class as well as removed legay session overview because we dont need it anymore.

I have one last suggestion for the sorting part.

Implemented it, works fine! Great advice, thanks!

Another cherry on top now, is that if you resize the window the session grid actually gets adjusted to it and does not use a fixed column delimiter of 3 anymore. It takes the width of the session objects and calculates how many of them fit into the current width of the scroll pane.