-
-
Notifications
You must be signed in to change notification settings - Fork 431
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
Introduced new ui.start bundle, which brings custom lifecycle status … #419
Conversation
…HTTP pages as well as an 404 error page. Signed-off-by: Kai Kreuzer <[email protected]>
Signed-off-by: Kai Kreuzer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaikreuzer thanks for picking this up, sounds like it could make it way more intuitive.
bundles/org.openhab.ui.start/src/main/java/org/openhab/ui/start/internal/RootServlet.java
Outdated
Show resolved
Hide resolved
try { | ||
pageStatus = IOUtils.toString(status.openStream()); | ||
} catch (IOException e) { | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho, the code could benefit a lot from a better separation between the error handling and the logic, what about not catching this error and marking the method to throw an IOException? If that is allowed otherwise splitting of some methods could be beneficial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the activation of the component, so we cannot declare catched exceptions.
And these errors really mean that the component isn't usable, so I think this should happen in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe splitting up the method could help but, but if at least the exceptions are cleaned 👍
Signed-off-by: Kai Kreuzer <[email protected]>
@martinvw Thanks for your review, all comments should be addressed! |
Maybe you should await the builds, I'll be offline :-) |
I understand that I should do the merge as the build is green as you cannot do it yourself - I hope this is what you meant ;-) |
openhab#419) * Introduced new ui.start bundle, which brings custom lifecycle status HTTP pages as well as an 404 error page. Signed-off-by: Kai Kreuzer <[email protected]> GitOrigin-RevId: 39f0e17
…HTTP pages as well as an 404 error page.
With this PR, openHAB will now show the following page (on any url) during startup as long as the dashboard is not yet available:
Once the system is up and running, any non-existent page will result in a 404:
This are first small steps to come towards eclipse-archived/smarthome#1896 and I think this alone is already quite useful.
Signed-off-by: Kai Kreuzer [email protected]