JBIDE-21720 Server Adapter: user choose default route when creating the server

[scabanovich]

No description provided.

[scabanovich]

@adietish , please consider. I also have a doubt if in the same method service parameter taken as getService() is better than this.service, because if the model was initialized with a service before it was loaded, updating with getService() will remove the initial value.

[fbricon]

this is not enough. the newRoute is ignored in

protected IRoute updateRoute(IRoute route, List<IRoute> routes, IService service) {
        if (!isLoaded) {
            return null;
        }

        if (routes == null
                || routes.isEmpty()) {
            route = null;
        } else {
            route = ResourceUtils.getRouteForService(service, routes);
            if(route == null 
                    || !routes.contains(route)) {
                route = routes.get(0);
            }
        }
        firePropertyChange(PROPERTY_ROUTE, this.route, this.route = route);
        return this.route;
    }

[scabanovich]

It would be nice if such cases could be found by the compiler. A warning like "The passesd value of parameter 'route' is always reasigned before it is used" might help.
I did not add check ResourceUtils.areRelated(route, service) because list to select a route contains all project routes.

[fbricon]

first thing to do in the method would be to

List<IRoute> matchingRoutes = ResourceUtils.getRoutesForService(service, routes);

then,

• replace routes by matchingRoutes
• remove route = ResourceUtils.getRouteForService(service, routes);

[fbricon]

What would be nice is tests catching these errors

[scabanovich]

There is method updateRoutes() that is called before updateRoute(). The right place to narrow the list of routes is updateRoutes(). Now its implementation calls getAllRoutes(service) that returns all routes for the project. @adietish , could you please confirm if this logic is correct?

[scabanovich]

I added test that for now checks the logic as is. We have to decide if the logic should be changed, then test may be updated to it.

[adietish]

currently writing tests for this model, and I also came across the fact that #getRoutes() returns all routes, while I keep wondering if we should only return the routes that match the currently selected service. getRoutes() is actually what the user gets as possible choices in the route combo. Does it makes sense that the user can choose whatever route he wants, even a route that's not related to the service that's currently selected?

[robstryker]

I think it's very strange that getRoutes() returns all routes, but it is just a model, and so it's not worth changing. I'd suggesting adding a new proper api method with javadoc that is getRoutesForService() with no argument, and possible a second with an IService attribute, which will return only the routes relevant to the selected (or passed in) service.

[robstryker]

Does it makes sense that the user can choose whatever route he wants, even a route that's not related to the service that's currently selected?

No. I don't this makes much sense from my PoV but maybe I don't have the same domain knowledge you do. I personally can't think up an example where he'd want to use one service and an unrelated route. But then I don't know all the places the routes are used aside from show in web browser / welcome page / whatever it's called now.

[adietish]

I am now merging this in after I have merged my tests & the change to the model which only returns the matching routes only (as discussed above): #1153

[adietish]

thx! merged into #1156