WIP Wrap JobProperty in optionalProperty for UI

[KostyaSha]

Refers to 3417dc0 where rowSet was set to true.
Should allow avoid optionalBlock wrappers in JobProperty implementations.

Property save doesn't work because DescriptorList binding is not fully simulated. Some jelly magic not fully generated...

[olivergondza]

What is the motivation behind this change? Why not just add the optionalBlock in config.jelly?

[KostyaSha]

@olivergondza because i tired fixing plugins that exposing all job properties into projects and builds.
jenkinsci/docker-plugin#201
jenkinsci/naginator-plugin#14
TODO GitHubJobProperty

This is also inconsistency for UI, all publishers/builder/wrappers are optional and only JobProperty bundled.

[daniel-beck]

It's not clear to me how this will improve the situation. Plugin authors can use optional blocks today already, and when they don't, why do you think they'll override a getter they don't need to?

[KostyaSha]

@daniel-beck it obvious that after @since TODO it will be possible to not implement wrappers and just mark Property as optional. Because this wrapping is not obvious for new developers and experienced are not reviewing changes (even i do this changes not from first try).

[jglick]

This is also inconsistency for UI, all publishers/builder/wrappers are optional and only JobProperty bundled.

This is true—if asked at the outset, I would have advised that JobProperty should be something you decide to add—but that is not how it works, and I fear that changing this now would be incompatible.

[KostyaSha]

@jglick that's why i'm adding optional field in property descriptor that set by default to the same behaviour as before.

[jglick]

Alternate suggestion, which I think would require no changes to existing code, only new options: introduce

/**
 * Job property which may or may not be present.
 * Must define {@code config-details.jelly} or {@code config-details.groovy}.
 */
public abstract class OptionalJobProperty<J extends Job<?,?>> extends JobProperty<J> {
    @Override
    public OptionalJobPropertyDescriptor getDescriptor() {
        return (OptionalJobPropertyDescriptor) super.getDescriptor();
    }
    public static abstract class OptionalJobPropertyDescriptor extends JobPropertyDescriptor {
        // constructors…
        @Override
        public JobProperty<?> newInstance(StaplerRequest req, JSONObject formData) throws FormException {
            return formData.optBoolean("specified") ? super.newInstance(req, formData) : null;
        }
    }
}

with an OptionalJobProperty/config.jelly like

<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:f="/lib/form">
    <f:block>
        <f:optionalBlock title="${descriptor.displayName}" name="specified" checked="${instance != null}" inline="true">
            <st:include page="config-details" from="${descriptor}" class="${descriptor.clazz}"/>
        </f:optionalBlock>
    </f:block>
</j:jelly>

[KostyaSha]

Closing because of cosmetic 2.0