Author Topic: New projects "Custom Vars" tab  (Read 13395 times)

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
New projects "Custom Vars" tab
« 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 :



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

Are you interested.

Regards.
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline BlueHazzard

  • Developer
  • Lives here!
  • *****
  • Posts: 3353
Re: New projects "Custom Vars" tab
« Reply #1 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...
« Last Edit: November 18, 2019, 08:41:37 pm by BlueHazzard »

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #2 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 )

* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline BlueHazzard

  • Developer
  • Lives here!
  • *****
  • Posts: 3353
Re: New projects "Custom Vars" tab
« Reply #3 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?

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #4 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
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 ?
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #5 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.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #6 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
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.
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #7 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.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #8 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.

(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #9 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 !
« Last Edit: November 20, 2019, 10:18:24 pm by earlgrey »
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #10 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?
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #11 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.

* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #12 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 :


( Maybe should I have written demonstrates ? )

 I think it is complete now.

Should I merge this feature in my master dev branch ?
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #13 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.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #14 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
const CustomVarHash& v = object->GetAllVars();
for (CustomVarHash::const_iterator it = v.begin(); it != v.end(); ++it)
with a loop like this :
Code
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
~ 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
~ Little helper ~
void CompileOptionsBase::VarEnumInit(CustomVarHash const * _i_hash) const
{
    m_VarEnumHash       =   _i_hash;
    m_VarEnumIterator   =   m_VarEnumHash->begin();
}

Code
~ 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
~ 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();
}
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline sodev

  • Regular
  • ***
  • Posts: 497
Re: New projects "Custom Vars" tab
« Reply #15 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.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #16 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?
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #17 on: December 01, 2019, 09:50:32 am »
CompileOptionsBase vars are now like this :
Code
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 :
  • without the flags extension
  • with two wxHashMaps, one for active vars, one for inactive vars
that does not lead to filtering a STL container ( coz there are 2 wxHashMaps ) may be preferable ?
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #18 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?
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #19 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
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
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.
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #20 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
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...
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #21 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
~ CompileOptionsBase.h ~
std::vector< CustomVar const & > & CompileOptionsBase::VarGetAll(int _i_flags = eVarActive) const;
Code
~ 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
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"
« Last Edit: December 01, 2019, 03:43:37 pm by earlgrey »
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13413
    • Travis build status
Re: New projects "Custom Vars" tab
« Reply #22 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)
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #23 on: December 01, 2019, 03:44:58 pm »
OK boss  ;D
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit

Offline earlgrey

  • Multiple posting newcomer
  • *
  • Posts: 101
Re: New projects "Custom Vars" tab
« Reply #24 on: December 04, 2019, 07:09:17 pm »
You can have a look @ https://github.com/earlgrey-bis/obuscated.cb/commit/2b4a0cfa2463fce9f3e0aac1b9791d5a7500f2aa

Missing :
- full & correct handling of keyboard shortcuts.
- some code cleanup ( comments, ... )
* OS = Debian Buster - Linux 4.19.06 x64 SMP
* C::B = svn11267 wx-3.0.4 - Linux, unicode 64 bit