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

[Feature Request]: Please remove "sealed" from KryptonWrapLabel and KryptonLinkWrapLabel #1023

Closed
MHartmann-427 opened this issue May 30, 2023 · 14 comments
Labels
awaiting feedback A fix for this issue has been implemented, waiting for feedback on the fix. completed This issue has been completed. enhancement New feature or request new feature A new feature has been requested. suggestion A suggestion has been requested.
Milestone

Comments

@MHartmann-427
Copy link

We are still using the original Krypton Toolkit coming from ComponentFactory/Phil Wright.
Because the original version written by Phil Wright has limitations and is not maintained anymore we plan to upgrade to Krypton-Suite/Standard-Toolkit.
Unfortunately we found one incompatibility to the original version, so we are not able to upgrade:
The control "KryptonWrapLabel" is implemented "sealed" in your version of Krypton ToolKit, so we can not inherit from this control (but we do).
Is there any need for sealing the class/control "KryptonWrapLabel"?

@MHartmann-427 MHartmann-427 added enhancement New feature or request new feature A new feature has been requested. suggestion A suggestion has been requested. labels May 30, 2023
@Wagnerp Wagnerp self-assigned this May 31, 2023
@Wagnerp Wagnerp added this to the Version 8 milestone May 31, 2023
@Wagnerp
Copy link
Contributor

Wagnerp commented May 31, 2023

Hi @MHartmann-427

Thanks for bringing this to our attention. I'll have a look into it as it might be an edit done by ReSharper. Will update you when it's ready for testing.

@Wagnerp Wagnerp changed the title Why KryptonWrapLabel control is implemented "sealed"? [Feature Request]: Please remove "sealed" from KryptonWrapLabel and KryptonLinkWrapLabel May 31, 2023
Wagnerp added a commit that referenced this issue Jun 4, 2023
@Wagnerp
Copy link
Contributor

Wagnerp commented Jun 5, 2023

Hi @MHartmann-427

Please try this feature in build >= .156

@Wagnerp Wagnerp added the awaiting feedback A fix for this issue has been implemented, waiting for feedback on the fix. label Jun 5, 2023
@Wagnerp
Copy link
Contributor

Wagnerp commented Jun 6, 2023

Closing in 30 days (06/07/2023) if no response is recorded.

@MHartmann-427
Copy link
Author

I tried to test the changes you made for this request, but unfortunately version 80.23.6.156-alpha is not compatible with original Krypton Toolkit coming from ComponentFactory/Phil Wright anymore.
The types "IPalette" and "KryptonPalette" do not exist, so it is not possible to update our application to version 80 of Krypton Toolkit.
In version 70 the missing types "IPalette" and "KryptonPalette" still exist.
It seems that there will be breaking changes between version 70 and 80?

@Wagnerp
Copy link
Contributor

Wagnerp commented Jun 12, 2023

I tried to test the changes you made for this request, but unfortunately version 80.23.6.156-alpha is not compatible with original Krypton Toolkit coming from ComponentFactory/Phil Wright anymore. The types "IPalette" and "KryptonPalette" do not exist, so it is not possible to update our application to version 80 of Krypton Toolkit. In version 70 the missing types "IPalette" and "KryptonPalette" still exist. It seems that there will be breaking changes between version 70 and 80?

Hi @MHartmann-427

In the upcoming v80 version, both IPalette & KryptonPalette have been modified to allow 'binary' serializations of custom themes, as documented here. Hope this helps :)

Edit: Sorry, wrong link, here is the correct one

@MHartmann-427
Copy link
Author

I replaced all occurrences of KryptonPalette with KryptonCustomPaletteBase and all occurrences of IPalette with PaletteBase as mentioned in the documentation link provided.
Good news: Our solution compiles correctly without errors now.
Bad news: We will get a NullReferenceException on method "Import" of class KryptonCustomPaletteBase, so I still can't see if the actual controls render correctly.

Here's the code we use to import a palette XML-file that is embedded in the resources of the assembly:

    private static KryptonCustomPaletteBase ReadPalette(string paletteName)
    {
        KryptonCustomPaletteBase palette = new KryptonCustomPaletteBase();
        string resourceName = $"wb.resource.resources.{paletteName}.xml".ToLower();
        Assembly asm = typeof(Resources).Assembly;
        string resource = asm.GetManifestResourceNames().FirstOrDefault(r => String.Compare(r.ToLower(), resourceName, StringComparison.Ordinal) == 0);
        if (!String.IsNullOrEmpty(resource))
        {
            Stream stream = asm.GetManifestResourceStream(resource);
            if (stream != null)
            {
                palette.Import(stream);
            }
        }
        return palette;
    }

Here's the stack trace of the NullReferenceException:
bei Krypton.Toolkit.PaletteBase.OnPalettePaint(Object sender, PaletteLayoutEventArgs e)
bei Krypton.Toolkit.KryptonCustomPaletteBase.OnPalettePaint(Object sender, PaletteLayoutEventArgs e)
bei Krypton.Toolkit.KryptonCustomPaletteBase.ResumeUpdates(Boolean updateNow)
bei Krypton.Toolkit.KryptonCustomPaletteBase.ResumeUpdates()
bei Krypton.Toolkit.KryptonCustomPaletteBase.Import(Stream stream, Boolean silent)
bei Krypton.Toolkit.KryptonCustomPaletteBase.Import(Stream stream)
bei WB.Basis.WBThemeManager.ReadPalette(String paletteName) in C:\WB-Projekte\DotNet\WB\trunk\WB.Basis\WBThemeManager.cs: Zeile203

@Wagnerp
Copy link
Contributor

Wagnerp commented Jun 13, 2023

Hi @MHartmann-427

Would it be possible to put together a sample project to figure out what's going on?

@MHartmann-427
Copy link
Author

MHartmann-427 commented Jun 13, 2023

Sample project attached :)

Meanwhile I surrounded the call of method "Import" with try-catch-block, but now I will get another NullReferenceException (next one comes from parameterless constructor of class NavigatorPage). I download the code of branch "alpha" and maybe I found the problem for all of the errors I will get.

The event invokation methods (like OnPalettePaint in class PaletteBase or OnTabStopChanged in class NavigatorPage) are not checking if the event is null:

    /// <summary>
    /// Raises the TabStopChanged event.
    /// </summary>
    /// <param name="e">An EventArgs containing the event data.</param>
    protected override void OnTabStopChanged(EventArgs e)
    {
        TabStopChanged.Invoke(this, e);
    }

It would be better to introduce NULL checking here like

    /// <summary>
    /// Raises the TabStopChanged event.
    /// </summary>
    /// <param name="e">An EventArgs containing the event data.</param>
    protected override void OnTabStopChanged(EventArgs e)
    {
        if (TabStopChanged != null)
        {
            TabStopChanged.Invoke(this, e);
        }
    }

KryptonToolkitTestApp.zip

@Wagnerp
Copy link
Contributor

Wagnerp commented Jun 13, 2023

Hi @MHartmann-427

Thanks for raising the null checking issue, will fix today :)

@Wagnerp
Copy link
Contributor

Wagnerp commented Jun 19, 2023

Hi @MHartmann-427

Please could you try again using build >= .170?

@MHartmann-427
Copy link
Author

The "Import" method of class KryptonCustomPaletteBase still crashing with NullReferenceException (now in method "OnButtonSpecChanged" of class "PaletteBase"): Here's the stack trace of NullRefrenceException:

bei Krypton.Toolkit.PaletteBase.OnButtonSpecChanged(Object sender, EventArgs e)
bei Krypton.Toolkit.KryptonCustomPaletteBase.OnButtonSpecChanged(Object sender, EventArgs e)
bei Krypton.Toolkit.KryptonCustomPaletteBase.ResumeUpdates(Boolean updateNow)
bei Krypton.Toolkit.KryptonCustomPaletteBase.ResumeUpdates()
bei Krypton.Toolkit.KryptonCustomPaletteBase.Import(Stream stream, Boolean silent)
bei Krypton.Toolkit.KryptonCustomPaletteBase.Import(Stream stream)

I donwloaded the code of current alpha branch and I see that you implemented null ckecking for event invokation of event "PalettePaint". But even in class PaletteBase there are four other events (AllowFormChromeChanged, BasePaletteChanged, BaseRendererChanged and ButtonSpecChanged) where still is no null checking on invoking those events. I think you have to check "all" events/invokation methods in "all" classes to ensure a proper null checking (seems to be a lot of work).
Null checking of event invokation is implemented well in version 70, so I wonder why it was removed for version 80.

The Standard-Toolkit" project is supported by JetBrains (don't know which product is used). With ReSharper (is used in our company) you can mark the event declarations with "CanBeNullAttribute" (from JetBrains.Annotations) and let ReSharper check for "Possible NullReferenceException".

@Wagnerp
Copy link
Contributor

Wagnerp commented Jun 19, 2023

But even in class PaletteBase there are four other events (AllowFormChromeChanged, BasePaletteChanged, BaseRendererChanged and ButtonSpecChanged)

Done, I'll let ReSharper have a look

@Smurf-IV
Copy link
Member

I donwloaded the code of current alpha branch and I see that you implemented null ckecking for event invokation of event "PalettePaint". But even in class PaletteBase there are four other events (AllowFormChromeChanged, BasePaletteChanged, BaseRendererChanged and ButtonSpecChanged) where still is no null checking on invoking those events. I think you have to check "all" events/invokation methods in "all" classes to ensure a proper null checking (seems to be a lot of work).

(seems to be a lot of work) - Yes it was, but this should be complete now. (see merges for #1020 )
@MHartmann-427 Please have a "g0" with the latest alpha

@Wagnerp
Copy link
Contributor

Wagnerp commented Aug 27, 2023

Hi @MHartmann-427

Please can you confirm that this is working in the latest canary build?

@Wagnerp Wagnerp removed their assignment Sep 4, 2023
@Wagnerp Wagnerp added the completed This issue has been completed. label Sep 4, 2023
@Wagnerp Wagnerp closed this as completed Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback A fix for this issue has been implemented, waiting for feedback on the fix. completed This issue has been completed. enhancement New feature or request new feature A new feature has been requested. suggestion A suggestion has been requested.
Projects
None yet
Development

No branches or pull requests

3 participants