Clover UI and Code improvement

[ITzTravelInTime]

Hi, since i started playing with clover's source code, i wanted to help by improoving the source code, mainly about the UI and the UI code which definelly needs work, so i have created this thread to discuss about this work i want to do.

 

First of all i am working on the menu.cpp file which is the file magaing clover's menus, this file (at the time of writing this post) is very crowded and packed with lots of stuff and the code quality isn't the best, the code is very repetitive and hard to work on, my objective is to improve the menu system and make easyer to work with clover's source in general (i also have some experience in the field of UI/UX other than coding in c/c++, so i can help not only with the code).

 

I also decided to ditch rewiting an ui engine from scratch (too labourious and the existing ui does the job) and i am oriented on refactoring existing work.

 

EDIT:

 

Some things that i think should be done are:

-Separating the menu.cpp file in multiple files to keep the code more organized

-Improoving variables and objects/types naming conventions

-Moove code with hardcoded UI strings to dedicated files

 

Ideas:

-Menu content and names revision

 

 

Edited November 6, 2020 by ITzTravelInTime

[Slice]

Menu.c was written many years ago step by step so why it looks like spagetti. If you have an idea how to do this more common way then welcome.

I may advice you to check how it work in QEMU. This is much simpler and not required to reboot.

I can help to tune the way.

[Jief_Machak]

Oups, I had created this one : 

I can't delete it, I think.

 

I'm currently refactoring some things :
  - xml parser : a new parser is already done and will be integrated into Clover, replacing almost all of Settings.cpp
  - converting string (CHAR16* and CHAR8*) to XString[8,W]
  - converting buffer manually allocated to XBuffer

  - is short : eliminating all Allocate(Zero)Pool and FreePool to get rid of all memory leak

 

I have other things in mind :
  - better separate the GUI and the functional objects. Separate the GUI engine.
  - create class for encapsulate EFI file functions (guarantee that no Close is forgotten).
  - create class for encapsulate installing and deinstalling protocols (guarantee that no installed stays when Clover quit).
  - create a mocking framework to be able to run Clover GUI as a macOs app.

 

Other ideas are welcome.

[ITzTravelInTime]

I have strted trying some code to improove the managemnt of ui elements beewteen the FillInputs and the ApplyInputs, so the old code for FillInputs looks like this:

 

UINTN InputItemsCount = 0;
    
  if (New) {
    //InputItems = (__typeof__(InputItems))A_llocateZeroPool(nControls * sizeof(INPUT_ITEM)); //XXX
    InputItems = new INPUT_ITEM[130];
  }

    
  InputItems[InputItemsCount].ItemType = ASString;  //0
  //even though Ascii we will keep value as Unicode to convert later
  // no need for extra space here, it is added by ApplyInputs()
  InputItems[InputItemsCount++].SValue.takeValueFrom(gSettings.BootArgs);
    
 InputItems[InputItemsCount].ItemType = UNIString; //1
  InputItems[InputItemsCount++].SValue.takeValueFrom(gSettings.DsdtName); // 1-> 2
    
  InputItems[InputItemsCount].ItemType = UNIString; //2
  InputItems[InputItemsCount++].SValue.takeValueFrom(gSettings.BlockKexts);

  InputItems[InputItemsCount].ItemType = RadioSwitch;  //3 - Themes chooser
  InputItems[InputItemsCount++].IValue = 3;

...

 

This is very repetitive and requires the array index to be always taken care of so I have written a C ++ class to avoid having to manually manage that while initializing all of those ui controls:

 

class MENU_SETUP{
    
private:
    
    INPUT_ITEM** items;
    
public:
    
    UINTN count;
    
    INPUT_ITEM* last(const BOOLEAN increment = TRUE){ //the pointed value will be modified, so no costant return pointer
        return &((*items)[increment ? count++ : count]);
    }
    
    INPUT_ITEM* addItem(const ITEM_TYPE type, const BOOLEAN increment = TRUE){
        ((*items)[count]).ItemType = type;
        return last(increment);
    }
    
    BOOLEAN lastValid(const BOOLEAN increment = FALSE){
        return last(increment)->Valid;
    }
    
    MENU_SETUP( INPUT_ITEM** controls, const BOOLEAN initialize = FALSE, const UINTN size = 1){
        count = 0;
        
        if (initialize){
            (*controls) = new INPUT_ITEM[size];
        }
        
        items = controls;
    }
    
};

NOTE that it will change because i am still finding some good way of implementing it

 

And so with this new class the same code inside FillInouts looks like this:

 

	MENU_SETUP menu = MENU_SETUP(&InputItems, New, nControls); 

//nControls is a static const with that hardcoded 130 value, static consts of numeric type are threted almost like macros and do not affect memory usage
    
    menu.addItem(ASString)->SValue.takeValueFrom(gSettings.BootArgs); //0
    menu.addItem(UNIString)->SValue.takeValueFrom(gSettings.DsdtName); //1
    menu.addItem(UNIString)->SValue.takeValueFrom(gSettings.BlockKexts); //2
    menu.addItem(RadioSwitch)->IValue = 3; //3

 

Which is mutch simpler and easyer to work with, for now i am starting converting the existing code to this new class then i will make something to remoove all hardcoded ui controls indexes in this way ui controls can be managed and added without having to modify the whole code, this will also lead to a mutch simpler way of adding new menus or modify existing ones and adding new controls.

 

This code is also somehwat inspired by the way some stuff is managed in swift apps.

 

Next step will be a revision of the menus layout/names to impoove usability and understandability by the final user, i have helped lots of people setting up hackintoshes in the past using clover and i know what is good and what is not about the current menu system, of course we will discuss changes first before applying them.

 

For now i am experimenting with this stuff on my own fork of Clover and hopefully you can see the whole code i worked on soon.

Edited November 6, 2020 by ITzTravelInTime

[ITzTravelInTime]

@Jief_Machak Some info:

 

The class INPUT_ITEM has different value variables which of which just one at a time seems to be used, if that's the case why not just put all of those inside an union? in this way you can save a lot of memory since you don't have to waste memory for variables you don't even use

Edited November 6, 2020 by ITzTravelInTime

[Jief_Machak]

Dynamic memory is not a big problem here.

Union should NOT be used, at all. Instead, objects with inheritance can be used. It's exactly what I did when I refactored plist and TagStruct.

 

5 hours ago, ITzTravelInTime said:

InputItems = new INPUT_ITEM[130];

INPUT_ITEM is already a class. So use XObjArray instead.

 

5 hours ago, ITzTravelInTime said:

menu.addItem(ASString)->SValue.takeValueFrom(gSettings.BootArgs); //0

menu.addItem(UNIString)->SValue.takeValueFrom(gSettings.DsdtName); //1

menu.addItem(UNIString)->SValue.takeValueFrom(gSettings.BlockKexts); //2

This code is smaller, but quite fragile. If you copy paste a line between 1 and 2, all indexes from 2 shift, and you still have to modify the rest of the code.

And also, when you have a hundred of line like this, you still have to count and be sure the index are the same in FillInputs and ApplyInputs.

To solve that, one way could be to have a map of key/value. Key could be an enum value, integer, or even a string constant (but that takes more memory), value will be the INPUT_ITEM.

For example :

menu.itemsArray.addItem(BOOT_ARG_MENU_ITEM_CODE, ASString)->SValue.takeValueFrom(gSettings.BootArgs); //I don't care which memory index

menu.itemsArray.getItem(BOOT_ARG_MENU_ITEM_CODE)

 

I think we can go a little further than that. Currently, REFIT_MENU_ENTRY_ITEM_ABSTRACT (historic name) holds a reference to one of the INPUT_ITEM in that global table. Why not having REFIT_MENU_ENTRY_ITEM_ABSTRACT encapsulate its own INPUT_ITEM instead ?

[ITzTravelInTime]

Yes, i know how fragile it is and i was carefoul to maintain orders as i was working on the code and i noticed how it depends on hardcoded item positions, so the best way maybe is to create an enum somewere (perhaps inside my custom class) with all the hardcoded positions into the InputItems buffer and remove the hardcoded integers which are all over the place and then edit the MENU_SETUP class i am working on as you said to use just indexes from the enum.

 

And so why not just letting the MENU_SETUP class do the job of taking care of the buffer all together? And so remooving the global buffer pointer and replacing it with a MENU_SETUP instance?

 

But the beast approach is to get rid of all of those idexes and to make the whole code index independant perhaps by adding pointers in each input_item to let it directly interface with the value it displays and has to set and using the value variables as temporary memory for the new value util the user applys the options. 

 

about REFIT_MENU_ENTRY_ITEM_ABSTRACT i get what you mean, but i haven't looked into that class yet

 

EDIT:

the old code was fragile in the same way as this new one as well, you could just copy pase a couple of lines to break everything

Edited November 6, 2020 by ITzTravelInTime

[Jief_Machak]

5 hours ago, ITzTravelInTime said:

perhaps by adding pointers in each input_item to

Pointers are rarely the best solution.

 

To be independent of the order of MENU_ITEM creation, you have to think by key instead of index.

 

One solution is this :

6 hours ago, Jief_Machak said:

To solve that, one way could be to have a map of key/value. Key could be an enum value, integer, or even a string constant (but that takes more memory), value will be the INPUT_ITEM.

For example :

menu.itemsArray.addItem(BOOT_ARG_MENU_ITEM_CODE, ASString)->SValue.takeValueFrom(gSettings.BootArgs); //I don't care which memory index

menu.itemsArray.getItem(BOOT_ARG_MENU_ITEM_CODE)

 

itemsArray (whatever name is better) wil contains tuple ( int, INPUT_ITEM ).

When we do

menu.itemsArray.addItem(BOOT_ARG_MENU_ITEM_CODE, ASString)->SValue.takeValueFrom(gSettings.BootArgs);

and let's say BOOT_ARG_MENU_ITEM_CODE == 999, you just add (999, INPUT_ITEM) to the array.

 

When we do 

menu.itemsArray.getItem(BOOT_ARG_MENU_ITEM_CODE)

you just browse the array and find the MENU_ITEM corresponding to 999.

 

That way, you are fully independent of the order the MENU_ITEM are created. That MENU_ITEM could be created first or last that doesn't matter.

 

 

6 hours ago, ITzTravelInTime said:

And so why not just letting the MENU_SETUP class do the job of taking care of the buffer all together? And so remooving the global buffer pointer and replacing it with a MENU_SETUP instance?

 

An object should replace the global buffer. Agreed.

 

6 hours ago, ITzTravelInTime said:

REFIT_MENU_ENTRY_ITEM_ABSTRACT

These are the objects that use MENU_ITEM. Having this class encapsulating a MENU_ITEM object instead of a reference will simplify the code, improve "self-sufficiency" and make the global buffer (or MENU_SETUP) totally useless.

It's ok to do the MENU_SETUP class. That'll improve things. I just want you to know and not be surprised or offended when MENU_SETUP will disappear when the refactoring of MENU_ITEM references held by REFIT_MENU_ENTRY_ITEM_ABSTRACT will be done.

 

This refactoring is something I've already done in BootloaderChoser. I didn't have time to put it on GitHub yet, but you can have a look.

 

6 hours ago, ITzTravelInTime said:

the old code was fragile in the same way as this new one as well, you could just copy pase a couple of lines to break everything

Absolutely.

rEFIt_UEFI.zip

[ITzTravelInTime]

34 minutes ago, Jief_Machak said:

When we do

menu.itemsArray.addItem(BOOT_ARG_MENU_ITEM_CODE, ASString)->SValue.takeValueFrom(gSettings.BootArgs);

and let's say BOOT_ARG_MENU_ITEM_CODE == 999, you just add (999, INPUT_ITEM) to the array.

 

When we do 

menu.itemsArray.getItem(BOOT_ARG_MENU_ITEM_CODE)

you just browse the array and find the MENU_ITEM corresponding to 999.

 

That way, you are fully independent of the order the MENU_ITEM are created. That MENU_ITEM could be created first or last that doesn't matter.

 

 

Just create a dictionary managed by some object then, it's probably the easyest way to tacle this problem, and also there should be a table with all of those item keys somehwere, for that i suggest to avoid macros because the pre-processor can have funny behaviours with many macros and to just use an enum instead, the enums let's also avoid dealiong with hardcoding values ourselfs and the compiler does all the job for us.

 

About that atteched zip file, what have you changed? i see thereisn't the spaghetti code inside menu.cpp anymore but where is it gone?

[Jief_Machak]

12 minutes ago, ITzTravelInTime said:

About that atteched zip file, what have you changed?

BootloaderChooser is much more simpler and only text based, so all the graphic part are removed. And a lot of other things. But have a look in REFIT_MENU_ENTRY_ITEM_ABSTRACT. It has its own INPUT_ITEM instead of a reference. Just like I said. That means the global is just not needed anymore. Looking at the code as I write, I forgot to remove that global array (the "INPUT_ITEM InputItems[130];"), But it's not used anymore.

No need for keys, no need for map or dictionary...

 

8 minutes ago, ITzTravelInTime said:

pre-processor can have funny behaviours

I never had problems with the pre-processor, but in that case, an enum will do it better, I agree.

[Jief_Machak]

PS : I really removed the array in this zip.

rEFIt_UEFI.zip

[ITzTravelInTime]

4 minutes ago, Jief_Machak said:

PS : I really removed the array in this zip.

rEFIt_UEFI.zip

 

This looks like a good starting point for a brand new graphic UI implementation from scratch, but as i said, it's too much work, but on the other hand working with the existing system is still a lot of work to do.

 

I want to mainly improve what's already there.

 

We should find a way to refactor the existing code to work just like the text-mode only code and removing all those stuff relying on syncing hardcoded values and repeat the same code over and over.

 

 

[Slice]

Abstract.

A programmer may write a loop like

for (int i=1; i<10; i++) {a=...}

but a compiler may make "unroll loop" and produce

a[1]=...;

a[2]=...;

....

In the Clover menu.cpp and settings.cpp are written like as "unroll loop".

This is not fine looking and very large but I have some advantages. I have individual approach to each line. I can make each setting own way with own default value and with own format which is not reachable in OC.

If you can make it better then I'll be happy.

[Jief_Machak]

A small thing : when building, the message appear after the step is done. That means that if an error happens, you're not sure in which step.

[SLINK] CLOVERX64
[DLINK] CLOVERX64
[OBJCOPY] CLOVERX64
make: *** [/JiefLand/5.Devel/Clover/Clover-projects/Clover--CloverHackyColor--master.2/Build/Clover/RELEASE_GCC53/X64/rEFIt_UEFI/refit/OUTPUT/CLOVERX64.efi] Segmentation fault: 11

Here, the problem happened in the step after OBJCOPY.

 

I modified build_rule.template. Now it'd be that :

[SLINK] CLOVERX64
[DLINK] CLOVERX64
[OBJCOPY] CLOVERX64
[GENFW] CLOVERX64
make: *** [/JiefLand/5.Devel/Clover/Clover-projects/Clover--CloverHackyColor--master.2/Build/Clover/RELEASE_GCC53/X64/rEFIt_UEFI/refit/OUTPUT/CLOVERX64.efi] Segmentation fault: 11

If I miss a good reason that the message was after instead of before, tell me.

 

Doing the above make me realise that the segmentation fault at compilation wasn't because of lto flag but coincidental to lto flag.

The reason is that with the lto flag, the sections wasn't exactly in the same order. GenFw was crashing because of a bug I've introduced in March when I was making live gdb to work. The funny part is that I just had to remove my modifications and put it back the way it was before ! Obviously, I was wrong.

Another example of the "real" solution hiding itself behind the visible one.

 

Don't forget to recompile BaseTools (make -C BaseTools) after having pulled.

 

 

 

Edited November 9, 2020 by Jief_Machak

[ITzTravelInTime]

On 11/7/2020 at 6:14 AM, Slice said:

In the Clover menu.cpp and settings.cpp are written like as "unroll loop".

 

This is not fine looking and very large but I have some advantages. I have individual approach to each line. I can make each setting own way with own default value and with own format which is not reachable in OC.

If you can make it better then I'll be happy.

 

The One big flaw in this code if that there is no real syncronization for hardcoded array indeces beetween the functions using those hardcoded indices to set and use UI elements and so copy pasting a single line of code in the wrong place can actually break the whole thing.

 

The class i have drafted does also allow for custom behaviour for each control (see the example code), but it's all more compact to make work easyer, i can add something to it to get rid of hardcoded indices and at least get rid of the copy-paste problem, it's as easy as creating and enum and refactoring some functions to get the right INPUT_ITEM using the enum instead of an hardcoded number, and also using the enum will also allow us to get rid of the hardcoded buffer size since it can just the value of the last item of the enum and using it as the buffer size, and so removing another weak point of the code.

 

It's probably the most doable way of improving the existing code without rewriting it substantially.

	menu.addItem(BoolValue)->BValue = gSettings.PatchNMI; //15
	menu.addItem(BoolValue)->BValue = gSettings.PatchVBios; //16
	menu.addItem(Decimal)->SValue.SWPrintf("0x%llX", gPlatformFeature); //17
	menu.addItem(Hex)->SValue.SWPrintf("0x%hX", gSettings.BacklightLevel); //18

	menu.addItem(Decimal, FALSE); //19
    
	if (gSettings.BusSpeed > 20000) {
 	   menu.last()->SValue.SWPrintf("%06d", gSettings.BusSpeed);
	} else {
 	   menu.last()->SValue.SWPrintf("%06llu", gCPUStructure.ExternalClock);
	}

	for (i=0; i<NGFX; i++) {
        
        const GFX_PROPERTIES* gfx = &gGraphics[i];
        
        menu.addItem(ASString)->SValue.SWPrintf("%s", gfx->Model); //20+i*6
        
        //21+i*6, 22+i*6
        switch (gfx->Vendor) {
            case Ati:
                menu.addItem(BoolValue)->BValue = gSettings.InjectATI; //21+6*i
                
                menu.addItem(ASString, FALSE); //22+6i
                if ( gSettings.FBName.length() > 2 ) { //fool proof: cfg_name is 3 character or more.
                    menu.last()->SValue.SWPrintf("%ls", gSettings.FBName.wc_str());
                } else {
                    menu.last()->SValue.SWPrintf("%s", gfx->Config);
                }
                break;
            case Nvidia:
                menu.addItem(BoolValue)->BValue = gSettings.InjectNVidia;
                for (j=0; j<8; j++) {
                    snprintf((CHAR8*)&tmp[2*j], 3, "%02hhX", gSettings.Dcfg[j]);
                }
                menu.addItem(ASString)->SValue.SWPrintf("%s", tmp);
                
                break;
            default: //intel hd and such
                
                //we should always consider more obscure gfx devices, which are not created by nor intel, nvidia or amd/ati
                menu.addItem(BoolValue)->BValue = gSettings.InjectIntel;
                menu.addItem(Hex)->SValue.SWPrintf("0x%08X", gSettings.IgPlatform);
                //continue;
                break;
        }
        
        //23+i*6
        menu.addItem(Decimal, FALSE);
        if (gSettings.VideoPorts > 0) {
            menu.last()->SValue.SWPrintf("%02d", gSettings.VideoPorts);
        } else {
            menu.last()->SValue.SWPrintf("%02d", gfx->Ports);
        }
        
        //24+i*6
        if (gfx->Vendor == Nvidia) {
            
            for (j=0; j<20; j++) {
                snprintf((CHAR8*)&tmp[2*j], 3, "%02hhX", gSettings.NVCAP[j]);
            }
            menu.addItem(ASString)->SValue.SWPrintf("%s", tmp);
            
        } else { //ATI and others there will be connectors
            menu.addItem(Hex)->SValue.SWPrintf("%08x", gfx->Connectors);
        }
        
        //25+i*6
        menu.addItem(BoolValue)->BValue = gfx->LoadVBios;
    }

	menu.count = 44; //should be a calculated value but we leave 44 because othwerwise it could brek the code
    
    menu.addItem(BoolValue)->BValue = gSettings.KextPatchesAllowed;//44
    menu.addItem(BoolValue)->BValue = gSettings.KernelAndKextPatches.EightApple;//45
    menu.addItem(BoolValue)->BValue = gSettings.KernelAndKextPatches.KPAppleIntelCPUPM;//46
    menu.addItem(BoolValue)->BValue = gSettings.KernelAndKextPatches.KPAppleRTC;//47
    menu.addItem(BoolValue)->BValue = gSettings.KernelAndKextPatches.KPKernelPm;//48
    menu.addItem(BoolValue)->BValue = gSettings.FixMCFG;//49

 

About other the existing code i have also to tell you to use use switch as mutch as you can when it's worth using, i have seen way too mutch useless if ... else if ... else if chains which just screams low quality/lazy code to me.

 

@Jief_Machak can you explain me the usage of XBuff to replace the usage of the hardcoded buffer?

 

BTW can i send you some design sketches to show you some ideas about making the clover UI better for users? Nothing revolutionary or difficoult to implement but just some ideas i have to help people having a better time using clover with GUI.

 

 

[Jief_Machak]

11 hours ago, ITzTravelInTime said:

About other the existing code i have also to tell you to use use switch as mutch as you can when it's worth using, i have seen way too mutch useless if ... else if ... else if chains which just screams low quality/lazy code to me.

I wouldn't agree. "else if" seems perfectly fine to fine. I'm pretty sure they compiled the same in the end.

And switch have this problems of having to break. Most people don't use block in each case, which means var declared in the first case can be accessible in the second.

That said, I'm happy if you prefer switch. Both are equally understandable to me.

 

I think we should get rid of the indexes.

 

Just declare "XObjBuffer<MyClass> myArray". Replace MyClass by the object class you want to store.

The more efficient way to add object is to allocate them "MyClass obj = new MyClass(ctor args)" and then store them "myArray.AddReference(obj, true)".

Access with operator [].

Example :

  Icons.setEmpty();
  for (INTN i = 0; i < BUILTIN_ICON_COUNT; ++i) { //this is embedded icon count
    XIcon* NewIcon = new XIcon(i, true);
    Icons.AddReference(NewIcon, true);
  }

 

We didn't talk about coding conventions. We don't have a lot as it seems unnecessarily annoying most of the time.

But : 

  - please adjust your editor to use 2 spaces instead tabs ( @Slice That's your setting, right ? If not, we should agree on one)

  - do not use automatic formatter. They never exactly do what you want. Avoiding as much as possible to change order of method in a file, or reformat a whole file because it changes a lot of line for git and it get harder to track changes.

  - NEVER use non-const reference as method parameter. If it's an IN parameter it's const reference, if it's an out, it's non-const pointer. An IN parameter that is an old C array (like CHAR8* or CHAR16*) will be const pointer, but they are supposed to be replaced.

  - I try to move towards smaller compilation units; Each one having a header of the exact same name.

  - we  should have started a readme about coding conventions, even if there isn't that much !!

[ITzTravelInTime]

14 minutes ago, Jief_Machak said:

I wouldn't agree. "else if" seems perfectly fine to fine. I'm pretty sure they compiled the same in the end.

And switch have this problems of having to break. Most people don't use block in each case, which means var declared in the first case can be accessible in the second.

That said, I'm happy if you prefer switch. Both are equally understandable to me.

 

To me the main advantage of the switch is that it will guarantee you that the code is going to jump just to the appropriate section of the block without repeting the branching operation for all the sections of the block, with if  ... else if ... on the other hand the way it's going to be handled depends on the compiler and the optimization settings, so in general switch is a better coding pracice, also because the code involves way less repetition which just helps in case of a bug (by letting you change just one line of code instead of seveal almost repeated ones and so eliminating the risk of forgetting to change something).

 

I know this sounds nitpickie but i am the kind of person who cares about code quality, i have been teached this stuff for years.

 

31 minutes ago, Jief_Machak said:

We didn't talk about coding conventions. We don't have a lot as it seems unnecessarily annoying most of the time.

But : 

  - please adjust your editor to use 2 spaces instead tabs ( @Slice That's your setting, right ? If not, we should agree on one)

  - do not use automatic formatter. They never exactly do what you want. Avoiding as much as possible to change order of method in a file, or reformat a whole file because it changes a lot of line for git and it get harder to track changes.

  - NEVER use non-const reference as method parameter. If it's an IN parameter it's const reference, if it's an out, it's non-const pointer. An IN parameter that is an old C array (like CHAR8* or CHAR16*) will be const pointer, but they are supposed to be replaced.

  - I try to move towards smaller compilation units; Each one having a header of the exact same name.

  - we  should have started a readme about coding conventions, even if there isn't that much !!

 

 

-About the editor tabs i understand that 2 spaces will make your code compact and help with really complex blocks of code to keep them small but i am not a fan of it, i think that more spaces or tabs really help with readability of the code, and in the first place you shouldn't make code so complex that needs.

 

-I like the automatic formatter in xcode, it's way i am used to code and uf your code is clean it will not make messes, the only thing i don't like about it is that it removes tabs from multiple line comments and that doesn't tab preprocessor directives (but this makes sense since the preprocessor doesn't work considering statements and code blocks)

 

-The non-const reference ban is a good programming rule, following the Principle of minimum permition and i agree with what you said.

 

-Smaller compilation units with headers named the same will simply work, the code files should have at max a few undreds of lines of code and be specialized in one job and only one, without cluttering them with code doing other stuff, for example, in this way of doing things, you should have a dedicated header and cpp file for each class and having the code using them divided into separate compilation units, according to the aim of the usage. I know it's a lot of work but it makes better to locate things in the code and to include into the headers just what you need (or clover to that) for each block.

 

-Establishing rules agreed by the people working on such a complex project is a must for me.

 

That said for a project like clover as-is the better option is: if ain't broke don't fix it

 

So focus on removing fondamental flaws of the system like the hardcoded indeces or the memory management issues and refactor things along the way if you need, and make all the new code using some good, agreed-on conventios.

 

 

[Jief_Machak]

2 minutes ago, ITzTravelInTime said:

To me the main advantage of the switch is that it will guarantee you that the code is going to jump just to the appropriate section of the block without repeting the branching operation for all the sections of the block, with if  ... else if ... on the other hand the way it's going to be handled depends on the compiler and the optimization settings, so in general switch is a better coding pracice, also because the code involves way less repetition which just helps in case of a bug (by letting you change just one line of code instead of seveal almost repeated ones and so eliminating the risk of forgetting to change something).

Sure. That's ok.

3 minutes ago, ITzTravelInTime said:

first place you shouldn't make code so complex that needs.

If you reformat files with a tabs, or 4 spaces, rules, it will be annoying in some editor (comparison editor, etc.) that doesn't handle space as well as some good editor.

I don't know if you ever had to merge 2 branches where all the sources were reformatted with a different ident rule. Not fun ! Although it was a while ago and more and more tools integrate a "ignore whitespace" in comparison.

I think the simplest is to have all the same rule about that. I guessed @Slice was using a 2 spaces and no tab rule, but I realise that I never asked. Maybe I'm wrong ! @Slice What's your preferences?

11 minutes ago, ITzTravelInTime said:

automatic formatter in xcode

In my experience, there's always something that goes wrong. A specific long line you want formatted in a specific way, a long if on multiple line you want column aligned, etc. Sometime I like '{' on the same line, sometime not, dependending on the context.

So please, do not format whole files. But of course, you're welcome to use it on code you create or re-work.

14 minutes ago, ITzTravelInTime said:

Principle of minimum permition

Didn't know the name of the principle. I just got that habit with my own rule : "I want to know, without reading the method I'll call, if that parameter could be modified !" 

 

We are mainly on the same page...

 

[ITzTravelInTime]

4 minutes ago, Jief_Machak said:

We are mainly on the same page...

 

 I did a little mistake, the Principle is actually called of "Principle of minimum priviledge" and it's stuff that's teached in universities nowdays, it simply states that a system has be allowed to have access to just what it needs to do and nothing more, so for exmaple if you don't need to change the value of something just get a constant reference to the value itself.

 

I had a coding teacher really focused on this stuff, in fact people did not pass the coding exam if they didn't use stuff currectly, it's crucial for the security of lots of system around the world and not respecting it is the cuase of many exploits and bugs.

 

[Jief_Machak]

I know the principle. For me it was "Principle of least privilege". I just didn't put that name on my const reference rule.

And 200% agree that this is a source of a lot of security problem. Monolithic kernel are just a nightmare. But we are still using them.

That principle dated back to... 1974 by Saltzer... That says a lot about how the industry really wanted things to be secure !

Have a look on the short video of that guy, explaining how OS will be so much more secure with this principle : 

 

[Slice]

Yes, this is Tiano code style requirement to use 2 spaces and never uses TAB. We must follow.

[Jief_Machak]

3 minutes ago, Slice said:

Yes, this is Tiano code style requirement to use 2 spaces and never uses TAB. We must follow.

I'm glad I guessed right. Yes, let's follow that.

[Slice]

About code style read please

https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C

and then

https://www.insanelymac.com/forum/topic/282787-clover-v2-instructions/?do=findComment&amp;comment=2721988

 

[ITzTravelInTime]

Understood, i will clone a fresh copy of the source and restart work from that, and this time complaining with the style you posted and without unnecessary modifications and adjustments like changing the tabbing

[ITzTravelInTime]

So after a long time thinking about what to do, i think the best solution is to ditch the hardcoded indexes entirely, i think that we might just make a function to create all of the ui controls and then have them belong to a class which has it's own refresh method and so each ui control must implement it's own data in order to refresh itself and so no longer having the refresh function refreshing all of the ui controls.

 

I think this is the best solution and will let the code avoid some of it's current major issues.

 

Also sorry for long absence guys, i had a lot of stuff going on in my life, including changing university, dealing with "the human malware" and more peronal stuff.