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

Automatically select first subcategory if a category is empty #86

Conversation

ArvidNy
Copy link
Contributor

@ArvidNy ArvidNy commented Aug 17, 2019

Fixes #83

@martinfrancois
Copy link
Contributor

Hi @ArvidNy
Can you please fix the style issues, so that the build passes again? Thanks!

@martinfrancois
Copy link
Contributor

martinfrancois commented Aug 19, 2019

I was just having a look through the PR... I have to say that while I see that it would make sense to skip empty categories entirely, even when selecting ones from the TreeView by the user, I don't know how good this would be in terms of usability: I could imagine someone for example in the extended demo clicking on Screen and wondering why it doesn't show up. The user could misinterpret it as a bug, asking themself why they can't access the screen category. It could also lead to bad habits, maybe a user expects, derived from this experience, that all top level categories don't have settings and misses out on categories that may have settings in them. Since there is no way for the user to differentiate "empty" categories from ones with settings in the TreeView in a way that is clear for the user, I don't know if this wouldn't confuse users.

I would propose: What if we just refactor the DEFAULT_CATEGORY into a property in the model (defaultCategoryPropety) and allow it to be set via a method in PreferencesFx like defaultCategory(...) that can be used for chaining, just as dialogTitle for example. Then, in the PreferencesFxModel you just have to change: setDisplayedCategory(getCategories().get(DEFAULT_CATEGORY)); to check if the property is set, and if it's not, it will take the first category as the default category (as it is now) and if the property is set, it will set that as the displayed category. What do you think? I think this would probably be the easiest and least invasive solution.

But I do agree that we should do something about the empty categories in general, since I agree it's not ideal (but I think this discussion would be something for a different issue & PR).

@ArvidNy
Copy link
Contributor Author

ArvidNy commented Aug 19, 2019

Sure, I can see that it might be confusing to some users that the main category will not open. We could of course revert the changes I made when clicking the breadcrumb and a category item and only make it behave this way if the default category is empty. That would still allow you to manually open the category and see that it is empty but it wouldn't be the first thing you see.

As for a method for setting the default category, I do agree that it would be very usable too.

@martinfrancois
Copy link
Contributor

This would also be a possibility. I would be okay with that!

@ArvidNy
Copy link
Contributor Author

ArvidNy commented Aug 19, 2019

Great, then I'll leave this PR open for you to merge - it think I've finally sorted out all the style issues now. If I find some spare time in the future I might open a new PR for setting the default category as well, but don't count on it as it looks now. So feel free to implement it yourself if you want to!

@martinfrancois
Copy link
Contributor

Thanks a lot! Looks good now, will merge it after the second check passed and will make a release 11.4.0

@martinfrancois martinfrancois merged commit 379aeec into dlsc-software-consulting-gmbh:master Aug 19, 2019
@ArvidNy ArvidNy deleted the select-first-subcategory-if-empty branch August 19, 2019 18:23
@martinfrancois
Copy link
Contributor

Thanks for your contribution! 8.4.0 and 11.4.0 should be available from Maven Central in about half an hour, but it will only appear in the search in a few hours.

@martinfrancois martinfrancois added the enhancement New feature or request label Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set default view/Show first subcategory
2 participants