Krypton-Suite / Standard-Toolkit

An update to Component factory's krypton toolkit to support .NET Framework 4.6.2 - 4.8.1 to .NET 6 - 8
BSD 3-Clause "New" or "Revised" License
399 stars 60 forks source link

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

Closed MHartmann-427 closed 1 year ago

MHartmann-427 commented 1 year ago

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"?

PWagner1 commented 1 year ago

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.

PWagner1 commented 1 year ago

Hi @MHartmann-427

Please try this feature in build >= .156

PWagner1 commented 1 year ago

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

MHartmann-427 commented 1 year ago

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?

PWagner1 commented 1 year ago

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 commented 1 year ago

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

PWagner1 commented 1 year ago

Hi @MHartmann-427

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

MHartmann-427 commented 1 year ago

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

PWagner1 commented 1 year ago

Hi @MHartmann-427

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

PWagner1 commented 1 year ago

Hi @MHartmann-427

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

MHartmann-427 commented 1 year ago

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".

PWagner1 commented 1 year ago

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 commented 1 year ago

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

PWagner1 commented 1 year ago

Hi @MHartmann-427

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