Code::Blocks

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: earlgrey on November 18, 2019, 08:15:53 pm

Title: New projects "Custom Vars" tab
Post by: earlgrey on November 18, 2019, 08:15:53 pm
Hello everyone.

I made ( for me ) a new "Custom vars" panel in the "Build options" configuration panel.
It looks like this :

(https://raw.githubusercontent.com/earlgrey-bis/erg.cbproject-custom-vars/doc/scr/cvars-19.11.16-001.png)

A minimalistic doc is at https://github.com/earlgrey-bis/erg.cbproject-custom-vars/tree/doc (https://github.com/earlgrey-bis/erg.cbproject-custom-vars/tree/doc)

Are you interested.

Regards.
Title: Re: New projects "Custom Vars" tab
Post by: BlueHazzard on November 18, 2019, 08:39:46 pm
Are this the variables for the global variables (Settings->Variables)? Or are this some custom variables? If custom: why not use a UI for the global variables? This was on my todo list for a long time.... Project specific global variables with notes... the project specific variables would only be used if no global variable is set...
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on November 18, 2019, 09:16:13 pm
Are this the variables for the global variables (Settings->Variables)?
No
Quote
Or are this some custom variables?
Yes, Project->Build options->Custom variables. It replaces the current "Custom variables" tab
Quote
If custom: why not use a UI for the global variables?...
I dont get you
==============================================================================================
Custom variables are per-project, saved in the project file ( .cbp ).
Global variables, Environment variables are attached to an OS user, saved in C::B configuration files ( .config/codeblocks/default.conf for me )

Title: Re: New projects "Custom Vars" tab
Post by: BlueHazzard on November 19, 2019, 11:47:52 am
I like this GUI.... I would add a "comment" or "note" column (also to the project file)

I am not quite sure what obfuscated thinks about the wxDataViewListCtrl, because in the past it was quite buggy on different platforms...

I also do not know if we want to add this before the next release, but in general i really like this! Can you create a patch?
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on November 19, 2019, 06:56:36 pm
I like this GUI.... I would add a "comment" or "note" column (also to the project file)
Should be possible with a thing like this in compileoptionsbase.h:
Code: [Select]
typedef struct
{
    wxString    a_value;
    wxString    a_comment;
} wxString2;
WX_DECLARE_STRING_HASH_MAP(wxString2, String2Hash);
And with a slight modification of all code parts that uses CompileOptionsBase::m_Vars. There are not too many apparently.

Quote
I am not quite sure what obfuscated thinks about the wxDataViewListCtrl, because in the past it was quite buggy on different platforms...
U right, that is the real problem

Quote
I also do not know if we want to add this before the next release, but in general i really like this! Can you create a patch?
Patches for each file are in the "pub" branch. There is a script for linux that apply the patches automatically.
Should I rather create a patch for the whole trunk ?
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on November 19, 2019, 09:01:37 pm
If you want to provide patches which could be reviewed it is best to make a git branch from a relatively new master (see my mirror: https://github.com/obfuscated/codeblocks_sf ).

I have no opinion on wxDVC probably it is too buggy in wx2.8 and wx3.0, but if its bugs doesn't affect the specific usage requirements for this case, then all is fine.

I'm not sure, but I guess I'd prefer if this was applied to the global variables and not to the custom variables. Also per workspace override should be possible, too.
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on November 20, 2019, 08:05:53 pm
If you want to provide patches which could be reviewed it is best to make a git branch from a relatively new master.
Done, a branch called erg.customvars-01-base is at https://github.com/earlgrey-bis/obfuscated.cb/tree/erg.customvars-01-base (https://github.com/earlgrey-bis/obfuscated.cb/tree/erg.customvars-01-base)
Quote
I have no opinion on wxDVC probably it is too buggy in wx2.8 and wx3.0, but if its bugs doesn't affect the specific usage requirements for this case, then all is fine.
Simple and basic usage of wxDVC, seems to works fine.
Quote
...I guess I'd prefer if this was applied to the global variables and not to the custom variables.
No, my goal is to modernize the CustomVars UI. CustomVars are very important because they avoid the proliferation of EnvVars.
Quote
Also per workspace override should be possible, too.
I dont understand, be more precise please.
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on November 20, 2019, 08:26:19 pm
No, my goal is to modernize the CustomVars UI. CustomVars are very important because they avoid the proliferation of EnvVars.
CustomVars are stored in the project, so they cannot contain per user information without making life hard for projects developed by more then 1 person.
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on November 20, 2019, 08:47:42 pm
Review comments:

General stuff:
1. no underscores in variable names
2. no too much alignment (I don't understand why people want to align unrelated if-then statements, but I guess this is just me)
3. no abbreviations dvlc and dvls doesn't mean too much if you don't know what are they in the first place
4. use range-based for loops whenever possible, but no auto variables...
5. no single line if statements

Patch specific:
1. Public apis must be documented, no matter that the surrounding apis aren't.
2. Why do you need a second map? Why don't you make a structure and add a bool? Or just add some kind of a set with inactive or active variables? Prefer hash based containers if you don't need sorting.
3. xrc files are generated by tools, don't edit them by hand and don't leave commented stuff in them
4. Why have you changed the names of the buttons? Add and Edit are pretty common in C::B?
5. You need to provide better commit message.
6. Why are the new api calls virtual?
7. These api calls must be exposed to scripting.

Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on November 20, 2019, 09:46:22 pm
Patch specific:
2. Why do you need a second map? Why don't you make a structure and add a bool? Or just add some kind of a set with inactive or active variables?
Because it keeps the m_Vars member intact for the rest of C::B code. Can be done ( and therefore add var comments ) but will change more C::B code.

3. xrc files are generated by tools, don't edit them by hand and don't leave commented stuff in them
Somebody give hints please for UI & .xrc modif please.

4. Why have you changed the names of the buttons? Add and Edit are pretty common in C::B?
"Clear" is not very comprehensive : does it delete the vars or just clear their content ? Or just one var ?
=> renamed to "Delete All"
=> Leads to keyboard shortcut problem ( D = "Delete", A = "Add", so what for "Delete All" ? )
=> "Add" renamed to "New" ( which is more appaired with "Delete" )
Current setup :
NEW -  BROWSE -  DELETE  - DELETE ALL
Tell what you want have.

6. Why are the new api calls virtual?
Because CompileOptionsBase:: methods related to m_Vars are. I asked myself the same question about them, because they are not overloaded anywhere.

7. These api calls must be exposed to scripting.
Somebody give hints please.


Its a lot of work, if you are really interested I can work on it.

[EDIT]
And thanx for your time and the review !
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on November 20, 2019, 10:19:38 pm
I don't see anything which prevents this from being merged, but it will have to wait for after the release.

BTW why are you using dataviewlistctrl? Why don't you use listctrl with checkboxes?
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on November 20, 2019, 10:59:55 pm
I don't see anything which prevents this from being merged, but it will have to wait for after the release.
So I have time to fit the codestyle compliance. And will use wxSmith to modify the xrc file.
Quote
BTW why are you using dataviewlistctrl? Why don't you use listctrl with checkboxes?
Was the same as with GTK : widget, model, renderers.

Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on November 24, 2019, 08:14:55 pm
This was on my todo list for a long time.... Project specific global variables with notes...
I got this working :

(https://raw.githubusercontent.com/earlgrey-bis/erg.cbproject-custom-vars/doc/scr/cvars-19.11.24-001.png)
( Maybe should I have written demonstrates ? )

 I think it is complete now.

Should I merge this feature in my master dev branch ?
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on November 25, 2019, 12:19:05 am
No, merges please. SVN doesn't support them. Just post a branch for review (just post the link to the branch here). If you don't want this to be forgotten open a ticket on sf.net about this, too.
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on November 27, 2019, 10:15:13 pm
Hello C::B devs. It is my intention to replace the old code for looping among CompileOptionsBase vars :
Code: [Select]
const CustomVarHash& v = object->GetAllVars();
for (CustomVarHash::const_iterator it = v.begin(); it != v.end(); ++it)
with a loop like this :
Code: [Select]
for ( CustomVarHash::const_iterator * it = object->VarEnumGetFirst(properties_flags) ; it != nullptr ; it = object->VarEnumGetNext() )
Motivation : adding active / inactive ( or any other ) property to vars leads to select the vars you want to enumerate by their properties. I want to keep this selection in CompileOptionsBase.
Would the following piece of code be accepted in C::B, because it has static vars in cpp module and it is not STL container enum. Thanx.
N.B. I am aware that m_Vars and m_ActiveVars can be merged.

Code: [Select]
~ in compileoptionsbase.cpp ~
//  these variables because in CB code, GetAllVars() is assumed const. So we cant define these vars as class members, since we modify them during the enumeration
static  int                                 m_VarEnumFlags;
static  CustomVarHash const             *   m_VarEnumHash;
static  CustomVarHash::const_iterator       m_VarEnumIterator;

Code: [Select]
~ Little helper ~
void CompileOptionsBase::VarEnumInit(CustomVarHash const * _i_hash) const
{
    m_VarEnumHash       =   _i_hash;
    m_VarEnumIterator   =   m_VarEnumHash->begin();
}

Code: [Select]
~ main search ~
CustomVarHash::const_iterator* CompileOptionsBase::VarEnumFind() const
{
    while ( m_VarEnumIterator != m_VarEnumHash->end() )
    {
        if ( m_VarEnumIterator->second.flags & m_VarEnumFlags )   ~ this is the test I want to keep in CompileOptionsBase ~
            return &m_VarEnumIterator;

        m_VarEnumIterator++;
    }

    if ( m_VarEnumHash == &m_InactiveVars )
        return nullptr;

    VarEnumInit(&m_InactiveVars);
    return VarEnumFind();
}

Code: [Select]
~ for(...) impl ~
CustomVarHash::const_iterator* CompileOptionsBase::VarEnumGetFirst(int _i_flags) const
{
    m_VarEnumFlags = _i_flags;

    VarEnumInit(&m_ActiveVars);
    return VarEnumFind();
 }

CustomVarHash::const_iterator* CompileOptionsBase::VarEnumGetNext() const
{
    m_VarEnumIterator++;
    return VarEnumFind();
}
Title: Re: New projects "Custom Vars" tab
Post by: sodev on November 27, 2019, 11:22:54 pm
C programmers writing C++ code...

I'm not affiliated with CodeBlocks but i have to drop my 0.5 cent here. Seeing an iterator that is passed around to a different entity is already looking strange, but seeing a pointer to an iterator is really weird. This is not how iterators are used, they are meant as a pointer replacement and usually are used value like together with their container in a close relationship.
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on November 30, 2019, 10:02:54 am
@earlgrey: Hm, your explanation is not enough. I don't understand what you want to achieve. You want to filter the variables by enabled/disabled? But why? What is the benefit?
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on December 01, 2019, 09:50:32 am
CompileOptionsBase vars are now like this :
Code: [Select]
typedef struct
{
    wxString    value;
    wxString    comment;
    int         flags;                    ~ active / inactive vars are just 2 bits in this field ~
} CustomVar;
Since the vars can be now of several categories ( with the flags ), whose active and inactive are just two possibilities, you may want to enumerate them following these categories : typically the original C::B code targets only active vars while the CompileOptionsDialog deals with other vars categories.

The difficulty is then to enumerate a subset of a STL container, by filtering properties of the objects it contains. And with a minimal API call code. It is perfectly possible that I am not enough skilled for doing this properly. Suggestions are welcome. ( I found only std::map<Key,T,Compare,Allocator>::equal_range and std::list<T,Allocator>::remove, remove_if, which dont fit )

The simpler implementation of active and inactive vars :
that does not lead to filtering a STL container ( coz there are 2 wxHashMaps ) may be preferable ?
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on December 01, 2019, 10:55:02 am
What is the goal? Performance? Ease of use?
Why don't you return a vector/array with the CustomVar struct?
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on December 01, 2019, 01:31:33 pm
> What is the goal ?
With the flags, you can extend vars properties in the future

> Performance ?
You have a  supplementary filtering code, so evidently less preformance

> Ease of use ?
Yes, one line code, and  CompileOptionsBase do the filtering, not the caller :
Code: [Select]
for ( CustomVarHash::const_iterator * it = object->VarEnumGetFirst(properties_flags) ; it != nullptr ; it = object->VarEnumGetNext() )

> Why don't you return a vector/array with the CustomVar struct ?
That is interesting ; I made that because I could not return the wxHashMap directly ; And I didnt want to build a filtered wxHashMap and return it ( heavy object, and the existing C::B code that calls GetAllVars() enumerates the keys, but dont uses the mapping feature). But a vector, why not ? Something like this :
Code: [Select]
virtual vector< CustomVar const & > VarGetAll(int _i_flags = eVarActive) const;
I just didnt think at it  >:(. I do this and I post on github so the code can be reviewed.

By the way the name "CustomVar" in the global namespace is not satisfying to me. CompileOptionsBase::Var would be logically OK but it is heavy ? Does someone have a replacement / suggestion to make.
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on December 01, 2019, 02:31:29 pm
> Performance ?
You have a  supplementary filtering code, so evidently less preformance
I doubt someone would make 100 or 1000 variables, so performance is not that important here.
If you want performance you'll leave filtering to the callers or use a bitset, but returning new objects on the stack is massive performance problem (new/delete/copy are really expensive functions).

Something like this :
Code: [Select]
virtual vector< CustomVar const & > VarGetAll(int _i_flags = eVarActive) const;
I just didnt think at it  >:(. I do this and I post on github so the code can be reviewed.
This API is really bad for performance...
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on December 01, 2019, 03:03:22 pm
> I doubt someone would make 100 or 1000 variables
I know that's why I did add the flags and the filtering.

> This API is really bad for performance...
Yes ; rather like this :
Code: [Select]
~ CompileOptionsBase.h ~
std::vector< CustomVar const & > & CompileOptionsBase::VarGetAll(int _i_flags = eVarActive) const;
Code: [Select]
~ CompileOptionsBase.cpp ~
static  std::vector< CustomVar const & > m_VarGetAllResult;
...
std::vector< CustomVar const & > & CompileOptionsBase::VarGetAll() const
{
    ~ filter the vars, fill m_VarGetAllResult, and return reference to it ~
}
The returned vector does not need to be const.

EDIT
This is wrong :
Code: [Select]
static std::vector< CustomVar const & > m_VarGetAllResult;
wont compile :

error: ‘struct std::_Vector_base<const CustomVar&, std::allocator<const CustomVar&> >::_Vector_impl’ has no member named ‘_M_finish’
  std::_Destroy(this->_M_impl._M_start, this->_M_impl._M_finish,
 "forming pointer to reference type"
Title: Re: New projects "Custom Vars" tab
Post by: oBFusCATed on December 01, 2019, 03:37:08 pm
I have no time to write the what is wrong with the last proposal, but this latest one is even worse.
Just leave the filtering to the client please.
Either you'll do it or I'll have to do it when I review your changes.  8)
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on December 01, 2019, 03:44:58 pm
OK boss  ;D
Title: Re: New projects "Custom Vars" tab
Post by: earlgrey on December 04, 2019, 07:09:17 pm
You can have a look @ https://github.com/earlgrey-bis/obuscated.cb/commit/2b4a0cfa2463fce9f3e0aac1b9791d5a7500f2aa (https://github.com/earlgrey-bis/obuscated.cb/commit/2b4a0cfa2463fce9f3e0aac1b9791d5a7500f2aa)

Missing :
- full & correct handling of keyboard shortcuts.
- some code cleanup ( comments, ... )