C++ proposition

[Jief_Machak]

Problem of theme switching is that InitTheme get the theme name from GlobalConfig.Theme (Settings.cpp:5204), but when switch theme, var ThemeX.Theme is updated (previous menu.cpp:792).

 

I've committed, but I commented out 2 lines. Modifying some XTheme var member to indicate to new wanted theme is not best practice because, for a small while, XTheme consistency is broken.

Having some globals var is not the best thing but ok. Just, instead of putting them in GlobalConfig, there should be in a struct which its only role would be to store the change the user selected in the menus.

But again, one at a time...

Edited April 5, 2020 by Jief_Machak

[Slice]

OK, accepted. Anyway XTHEME support for PNG themes is still not good so I can't finally switch to xtheme now.

[Jief_Machak]

Did you do something about the 2 commented lines ?

[Slice]

9 minutes ago, Jief_Machak said:

Did you do something about the 2 commented lines ?

These variables will be set later while parsing theme.plist or theme.svg.

[Jief_Machak]

That's better. Every is deducted from theme name. Good.

[Jief_Machak]

In Settings.cpp, I found this :

  Prop = GetProperty (DictPointer, "Arguments");
  if (Prop != NULL && (Prop->type == kTagTypeString)) {
    if (Entry->Options != NULL) {
      FreePool (Entry->Options);
    } else {
      Entry->Options = PoolPrint (L"%a", Prop->string);
    }
  }

Looks like a bug, because if Entry->Options != NULL , it's just freed and not assigned, even to NULL. Looks like it should be :

  Prop = GetProperty (DictPointer, "Arguments");
  if (Prop != NULL && (Prop->type == kTagTypeString)) {
    if (Entry->Options != NULL) {
      FreePool (Entry->Options);
    }
    Entry->Options = PoolPrint (L"%a", Prop->string);
  }

Do you agree ? If yes, don't change it, I'm currently refactoring to use XString.

 

With XString, it could just be 

  Prop = GetProperty (DictPointer, "Arguments");
  if (Prop != NULL && (Prop->type == kTagTypeString)) {
      Entry->Options.SPrintf("%s", Prop->string);
  }

which seems more logical. Because it's working like that, I don't want to change behaviour if I misunderstood.

[Slice]

Agree. I will wait your next commit to not mix sources.

[Jief_Machak]

I'm testing compilation (XCODE, GCC53 and VS2017 ). It will be in few minutes now.

[Jief_Machak]

Always longer than expected, with computer :-)

Done.

[Slice]

I think InitSelection is wrong cause it will not reinit selections after theme switch.

[Jief_Machak]

We can do "SelectionImages[0].setEmpty".

But I think you should consider change ThemeX to a ptr, and delete and recreate when switch theme. That way, when loading the theme, you are always in the same known state.

But we can also write a "XTheme.setEmpty()". That should not be hard, and will avoid to destroy and reallocate all the memory for XString, XImage, XArray etc. This solution has this nice advantage, but the big drawback of making impossible to separate themes in subclasses (ThemeSVG, ThemePNG, ThemeEmbedded <- that can be a subclass of ThemePNG).

 

I'll probably go for the first solution : delete and recreate.

[Slice]

I see no more bugs with XTHEME support, do you?

[Slice]

Oops! OS icons absent both in SVG and PNG themes.

[Jief_Machak]

Yes, true. LoadOsIcon is non existant when USE_XTHEME == 1

[Slice]

1 hour ago, Jief_Machak said:

Yes, true. LoadOsIcon is non existant when USE_XTHEME == 1

I made it. Committed.

[Slice]

Almost good. But one note. If APFS icon absents then we should choose vol_internal as priority not embedded as it is now.

[Slice]

What may be a caveat to switch DrawText from XStringW to XString? Are there same functions like StrCmp, StrStr?

[Slice]

On 4/4/2020 at 6:44 PM, Jief_Machak said:

That's an interesting mistake. I did this :

VOID REFIT_MENU_SCREEN::AddMenuInfoLine(IN XStringW& InfoLine)
{
  InfoLines.AddReference(&InfoLine, true);
}

which results to store a reference in a container (XObjArray). Problem is that Infoline will probably be destroyed at some point (only case it would not is if the caller supply a reference to a global object).

Long story short : don't do what I did !!!

Rule of thumb : never put a reference to a parameter in a container/collection. 

Is it good for Icon collection?

Icons.AddReference(NewIcon, true);

[Jief_Machak]

If NewIcon is a newly manually allocated object, yes it is.

 

Was just pointed out that, to put a referenced object in a collection, it has to be "owned" version, or else, we'll have to make a copy.

 

You were doing this 

    Icon NewIcon(i); //initialize with embedded but further replace by loaded
    ParseSVGXIcon(mainParser, i, NewIcon.Name, Scale, NewIcon.Image);
    ParseSVGXIcon(mainParser, i, NewIcon.Name + "_night", Scale, NewIcon.ImageNight);
    Icons.AddCopy(NewIcon);

which is fine IF the copy operator would have been defined in Icon object.

But a copy is one memory allocation more, so I changed it to

    Icon* NewIcon = new Icon(i); //initialize with embedded but further replace by loaded
    /*Status = */ParseSVGXIcon(mainParser, i, NewIcon->Name, Scale, &NewIcon->Image);
    /*Status = */ParseSVGXIcon(mainParser, i, NewIcon->Name + "_night"_XS, Scale, &NewIcon->ImageNight);
    Icons.AddReference(NewIcon, true);

 

The things NOT to do is 

    Icon NewIcon
    ...
    Icons.AddReference(NewIcon, true);

or

    a_method(const Icon& icon)
    {
        an_objarray.AddReference(icon, true);
    }

(which is the equivalent of my mistake with XString).

 

because Icons will keep a reference on a object that will be destroyed by C++ at the end of the block (or later if it's a parameter).

 

So yes, it's fine.

[Slice]

I probably close all xtheme issues and so I set USE_XTHEME=1 to be default.

But I still don't cleanup old codes for more carefully testing few days. And then yes, we may cleanup.

 

About Icon class there is still my misunderstanding how to operate with it. Once new Icons is created I can't change it? See my new commit...

[Jief_Machak]

Sorry, I'm not sure what you mean, and don't have much time today. If you refer to the problem of few days ago :

Yes, you can change them. At theme reload, instead of emptying the ArrayObj of Icon, you can browse it and do Icons = something. Or give that object to a method. It would be more efficient, for sure.

Not sure it answer the question... Can you be more specific ? Tell me which line of code has some problem ?

 

[Slice]

Procedure

const XImage& XTheme::GetIcon(INTN Id) const

if Icons.Image is absent then return embedded. It works.

Then I want to save embedded image into the Icons.Image to not do the works everytime the icon needed and any method fails with compiler claims something about "const". I didn't understand who is const.

[Jief_Machak]

I git pulled, but it's compiling fine.

But I have a guess : this method "const XImage& XTheme::GetIcon(INTN Id) const" is declared const (the const at the end), which means that this method doesn't modifiy the object it belongs to. If you want that method to store something, it means you'll modify the object, therefore you need to remove the const at the end. 

[Slice]

46 minutes ago, Jief_Machak said:

I git pulled, but it's compiling fine.

But I have a guess : this method "const XImage& XTheme::GetIcon(INTN Id) const" is declared const (the const at the end), which means that this method doesn't modifiy the object it belongs to. If you want that method to store something, it means you'll modify the object, therefore you need to remove the const at the end. 

Thanks, it is that I will know.

 

Sometimes I have a crash. How to catch a reason?

73:513  0:001  change theme
73:514  0:001  === [ InitXTheme ] ==============================
73:515  0:001  use daylight theme
73:535  0:019  Using theme 'ios7' (EFI\CLOVER\themes\ios7)
73:537  0:001  chosen theme ios7
73:539  0:002  OS main and drive as badge
74:794  1:255  file sound read: sound.wav Not Found
74:812  0:017  Loading font from ThemeDir: Success
74:892  0:080  XArray::ElementAt(xsize) -> Operator [] : index > m_lenA fatal error happened. System halted

[Jief_Machak]

Remember the gdb_launch I've created in Qemu folder ? Go in this Qemu folder and ./gdb_launch [your efi file].

You can use that and you'll be the backtrace in case of a crash or a panic. Remember to disable to unit tests because they generate panic intentionnally.

 

When you use gdb_launch, you can pass the path of your efi file. A file .debug must exist next to the efi. Default is ../Build/Clover/DEBUG_GCC53/X64/CLOVERX64.efi

 

At first launch, disk_image_gpt.img.zip will be uncompress. You can modify it as much as you want. It's an ignored file for git, so no risk of clutter.

Don't forgot to update your gdb with brew and sign it.