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

Fix #11421: Ignore ColumnRenderer helper in components using columns #11805

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

melloware
Copy link
Member

Fix #11421: Ignore ColumnRenderer helper in components using columns

This PR does it the "better" way with a utility to wrap and restored the helper. Note we need to do this on all components that have custom "Column Rendering".

@melloware melloware added 🐞 defect Bug...Something isn't working discussion Item needs to be discussed by core devs labels Apr 24, 2024
@melloware
Copy link
Member Author

also i didn't know if "Stateless" was the right term since this is removing a value from the State map...

@tandraschko
Copy link
Member

i like this more actually but it wraps IOException into FacesException, which can have drawbacks on exception handling propably
but not sure

@Rapster
Copy link
Member

Rapster commented Apr 25, 2024

Not sure about the name, but the try/catch and the return null doesn't look nice... Use a Runnable instead, something like FailableRunnable for nicer exception handling

@melloware
Copy link
Member Author

@Rapster can you give an example of how that would work? I have only use Runnable's for threading not just for a lambda wrapper?

@Rapster
Copy link
Member

Rapster commented Apr 25, 2024

You can use Runnable just like any other functional interfaces

public static <E extends Throwable> void runInRequestScope(FacesContext context, String var, Object value, FailableRunnable<E> runnable) throws E {
    // if no var passed just execute the callback
    if (LangUtils.isBlank(var)) {
        runnable.run();
        return;
    }

    Map<String, Object> requestMap = context.getExternalContext().getRequestMap();

    Object oldValue = requestMap.put(var, value);
    try {
        runnable.run();
    }
    finally {
        if (oldValue != null) {
            requestMap.put(var, oldValue);
        }
    }
}

@melloware
Copy link
Member Author

OK @Rapster i think I got it if you want to re-review

@melloware melloware force-pushed the PF11421-util branch 9 times, most recently from 5e7d8fc to 34c099f Compare April 25, 2024 22:03
@melloware melloware removed the discussion Item needs to be discussed by core devs label Apr 25, 2024
@tandraschko
Copy link
Member

tandraschko commented Apr 26, 2024

why not just a callbacks package? thats the more common way

@Rapster
Copy link
Member

Rapster commented Apr 26, 2024

As they're just extension of java.util, it makes sense to keep them in org.primefaces.util. I don't like having a deep and wide package structure, it always end up having a mess: the flatter the better.

Callbacks at this point is just a container, but could become what Failable is (from commons-lang), we'll most probably introduce new utils in the future, and no need to create another utils

One may argue how cumbersome it is to do someting like Callbacks.FailableConsumer but you can use static import and that should solve the "problem"

@tandraschko
Copy link
Member

all right, thats ok for me

static imports are even more mess in the most times, so lets better stay with it

@melloware
Copy link
Member Author

melloware commented Apr 26, 2024

so I am OK to merge this?? I am very happy with this PR with @Rapster help where we landed on this!

@melloware
Copy link
Member Author

@Rapster how about runWithoutStateVariable since that is really what its doing?

@Rapster
Copy link
Member

Rapster commented Apr 26, 2024

i'm fine with that

@tandraschko
Copy link
Member

tandraschko commented Apr 26, 2024

not sure about the name
its not a "state" var actually - "state "in JSF is always viewstate
better would be something like request attribute or FacesContextVar, FacesContextScopedVar

@melloware
Copy link
Member Author

I changed it to runWithoutFacesContextVar

@melloware melloware merged commit 21319e9 into primefaces:master Apr 26, 2024
12 checks passed
@melloware melloware deleted the PF11421-util branch April 26, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 defect Bug...Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SelectCheckboxMenu: Custom rendering broken when used as DataTable filter in a ColumnGroup
3 participants