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

Messages showDetail has wrong default value and shows summary twice #352

Closed
zhedar opened this issue Apr 25, 2016 · 13 comments
Closed

Messages showDetail has wrong default value and shows summary twice #352

zhedar opened this issue Apr 25, 2016 · 13 comments
Assignees
Milestone

Comments

@zhedar
Copy link
Collaborator

zhedar commented Apr 25, 2016

The documentation states, that showDetail should be false, but the real value is true.
Therefore currently the summary message will be shown twice at the moment, if no detail is provided as
StackOverflow user b.lopes reports.

This probably shouldn't happen at all, maybe we should opt for not showing a detail, if it's value is null.

I have not checked this, but the same may apply to the growl.

@stephanrauh
Copy link
Collaborator

I've corrected the default values in the jsfdsl file. Three components are affected: b:message, b:messages and b:growl.

@stephanrauh stephanrauh added this to the v0.8.2 milestone Apr 26, 2016
@stephanrauh stephanrauh self-assigned this Apr 26, 2016
@zhedar
Copy link
Collaborator Author

zhedar commented Apr 26, 2016

Thanks, I had that sitting in my trunk but didn't get to it yet. What do you think about checking the detail message for null (or empty) additionally to just checking showDetail, so this won't happen?
Edit: Although now with the correct value of showDetail, that isn't that urgent. We may check for the summary being null instead, thus showing the detail.

@stephanrauh
Copy link
Collaborator

Actually, I don't get it. Why is the summary shown twice if the detail is null? Looking at the code, I'd expect either nothing or the text "null":

                if (message.isShowDetail()) {
                    rw.startElement("span", component);
                    writeAttribute(rw, "class", "bf-message-detail");
                    if (message.isEscape()) {
                        rw.writeText(msg.getDetail(), null);
                    } else {
                        warnOnFirstUse();
                        rw.write(msg.getDetail());
                    }

                    rw.endElement("span");
                }

@zhedar
Copy link
Collaborator Author

zhedar commented Apr 26, 2016

Did you verify the bug in the current snapshot though?

I remember, that PrimeFaces' growl had (or has?) the same issue. Maybe UI-message sets detail = summary, if detail is null? Maybe we can check for equality between summary and detail and show only one of them, if they are equal?

@asterd
Copy link
Collaborator

asterd commented Apr 29, 2016

This bug was solved by the cc35075 commit.
Can you test it to check if now is all ok?

@stephanrauh
Copy link
Collaborator

I've changed the default value back to "true". Suppressing the details of an error message by default seems to be a bit odd. However, I've corrected the documentation.

@zhedar
Copy link
Collaborator Author

zhedar commented Apr 29, 2016

@stephanrauh Saw that, just wanted to write that we may need a fix for the original problem now again:
bildschirmfoto 2016-04-29 um 23 23 18
Taken from the showcase. Seems like the infamous icons.css bug is back again, I just don't know why yet.

However, I may have an idea considering the double messaged, will test that now.

Edit: I currently cannot reproduce the icons.css bug, but I got a lot of Unable to save dynamic action with clientId warnings. Maybe my build isn't up to date... need to check that :/
Edit 2: OK, I could fix some of these errors, but some of them come from the BootsFacesWeb project itself h:outputStyleSheet is producing this. I may check that on another project and report back in the corresponding issue.

@stephanrauh
Copy link
Collaborator

Actually, I prefer duplicate message over no messages. But I agree with you that we should avoid these duplicate messages. They are a standard behavior of JSF, but that's no excuse: we've founded BootsFaces to make the JSF world a better place :).

@stephanrauh
Copy link
Collaborator

The duplicate message shows only after a full-page request (F5 key). Plus, the "icons" message doesn't pop up on my local installation.

@zhedar
Copy link
Collaborator Author

zhedar commented Apr 30, 2016

As I have mentioned, I couldn't reproduce the icons.css bug as well. However the initial messages show only once for me, refreshing the page via cmd + R doesn't work, neither in Safari nor Chrome, although the cache is disabled. I didn't figure out, why this is yet. Therefore this is currently quite hard to reproduce for me.
OK, the CheckboxBean is SessionScoped, that's why the message is shown only once. I will change this to ViewScoped to test. Are there any reasons, why this should be SessionScoped anyway?

Edit: I think I got this. Currently testing a bit and reviewing the existing code.

@zhedar zhedar closed this as completed in 5db497a Apr 30, 2016
@zhedar
Copy link
Collaborator Author

zhedar commented Apr 30, 2016

The problem should be fixed my now.
Now we don't really need the user to set showSummary or showDetail explicitly, It should always be rendered correctly. If only a summary is provided, I stayed with our current implementation: Show it as a detail, because if there is only a single message, we don't need to format that bold, it looked a bit weird. If only a detail is provided, the text won't be indented anymore.

@stephanrauh
Copy link
Collaborator

That's great news! Unfortunately, the showcase still shows the "icons.css" messages on first load:
image
I've tested it with Google Chrome on OS X (but I'm sure it's not a browser problem).

@zhedar
Copy link
Collaborator Author

zhedar commented May 1, 2016

@stephanrauh I found the problem: We're both using the maven build. However, the gradle build doesn't remove resources from the mavenResources folder, it will only replace existing ones.
So somehow the gradle build changed and icons.css won't get copied into the theme folders anymore and is therefore missing in the showcase.

As we're adding AddResourcesListener.addThemedCSSResource("icons.css"); the resource will be registered as css/theme/icons.css instead of css/icons.css. I just fixed that in 0810307 by putting in an additional check.

I guess as of now we won't need the additional icons.css files in the theme folders anymore :)

@zhedar zhedar mentioned this issue May 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants