Handling of illegal values in settings views is not very intuitive but I don't know whether sth could really be done there...

[JonasSchaub]

I am specifically referring to integer and double settings. If an illegal input has been made, the user is told about it. The text field is then reset to 0 or 0.0 but actually, the settings remains in its previous value which is not displayed.

[JonasSchaub]

We could think about supplying possible values for the settings instead of having text fields. E.g. for the "nr of tasks for fragmentation" setting, this would be a nice solution.

[FelixBaensch]

What is meant by illegal values? Wrong data type or values outside a defined range?

[FelixBaensch]

We could think about supplying possible values for the settings instead of having text fields. E.g. for the "nr of tasks for fragmentation" setting, this would be a nice solution.

To stay with the example, a CPU with 64 possible cores should be offered a selection from 1 to 64?

[JonasSchaub]

What is meant by illegal values? Wrong data type or values outside a defined range?

Values outside a defined range. E.g. try to set the "nr of tasks for fragmentation" to a value higher than your available cores to see what I mean.

To stay with the example, a CPU with 64 possible cores should be offered a selection from 1 to 64?

Exactly. But for some fragmenter settings, it will not be that easy.

[FelixBaensch]

What is meant by illegal values? Wrong data type or values outside a defined range?

Values outside a defined range. E.g. try to set the "nr of tasks for fragmentation" to a value higher than your available cores to see what I mean.

How about coloring the background or the border of the text field red, adding an error tooltip and deactivating the "Apply" button?

To stay with the example, a CPU with 64 possible cores should be offered a selection from 1 to 64?

Exactly. But for some fragmenter settings, it will not be that easy.

Strange and complicated, imho.

[JonasSchaub]

The main problem here is that currently, the JavaFx property object throws an IllegalArgumentException if an illegal value was set which resets the binding but can't really be caught except by the uncaught exception handler. So at the moment, I don't know how to react in the GUI to the illegal value.

[FelixBaensch]

I could take a look, but it will take a while.

[JonasSchaub]

That would be great but I also had the idea to set a student onto this. Anyway, I don't think we should wait for this with the next release.

[FelixBaensch]

That would be great but I also had the idea to set a student onto this. Anyway, I don't think we should wait for this with the next release.

If you have a capable student.

Btw. funny copy&paste error in SettingsContainer line 649:

this.recentDirectoryPathSetting = new SimpleStringProperty(this,
                "Recent directory path setting",
                SettingsContainer.RECENT_DIRECTORY_PATH_SETTING_DEFAULT) {  
            @Override
            public void set(String newValue) throws IllegalArgumentException {
                if (SettingsContainer.this.isLegalRecentDirectoryPath(newValue)) {
                    super.set(newValue);
                } else {
                    IllegalArgumentException tmpException = new IllegalArgumentException("An illegal number of tasks for fragmentation was given: " + newValue);
                    SettingsContainer.this.LOGGER.log(Level.WARNING, tmpException.toString(), tmpException);
                    //no GUI alert here because this is an internal setting
                    //re-throws the exception to properly reset the binding
                    throw tmpException;
                }
            }
        };

[JonasSchaub]

Fixed in https://github.com/FelixBaensch/MORTAR/pull/60 thanks for spotting this!

[FelixBaensch]

This turns out to be more complex, as the validation is handled in the SettingsContainer and there is no (elegant) way to access the text field from there.

[JonasSchaub]

Precisely...

[FelixBaensch]

... a major workaround is necessary and the structure as it is now with SettingsView, -Controller and -Container cannot remain as it is. I would therefore not change it for the time being.

[JonasSchaub]

A major rework like this was not my intention "for the time being".