Fix for issues #550 and #553

[andresmmera]

This PR fixes both #550 and #553

After doing some research, I've found that the underlying problem in #550 was that the default symbol of the component's library was not loaded after using the searchbox. Specifically, the IRF7413 doesn't come with a symbol, so the NMOSFET's default symbol must be used. However, the tool was not loading it, so 'DefaultSymbol' was empty and, consequently, the preview symbol was not being updated at all:

https://github.com/Qucs/qucs/blob/master/qucs/qucs-lib/qucslib.cpp#L513

In this sense, I've simply added a function for loading the default library symbol after selecting a short-listed component.

On the other hand, #553 shows that QucsLib was copying to clipboard a wrong component (only when the component was shortlisted, I mean). The solution to this issue was to update the component name/library after the component was selected.

[felix-salfelder]

this functionality should be part of the symbol/component class that you are experiencing problems with.

please relocate (do not add) void QucsLib::LoadDefaultSymbol(QString libpath) and/or explain more, so i can help with this.

we should move towards proper symbols that look like symbols from the library core, and behave like symbols. symbols dont need to be loaded again, once they are there.

Specifically, the IRF7413 doesn't come with a symbol

i think, unfortunately, the IRF thing is a COMPONENT (correct?). one way to get out of this will be be to think of COMPONENTs as the future SYMBOLs. so until we do have symbols, each component must pretend to be a symbol...

I've simply added a function

the harder part is to get rid of them later. so i'm against adding it. i'm sure it's not required.

[andresmmera]

According to your suggestion, I removed void QucsLib::LoadDefaultSymbol(QString libpath) and placed its code where this function was called.

I'll try to elaborate a bit more:

About #550: So far, these parts collected in a library file (x.lib) should contain a symbol (*) and a component model. However, as I could see, many components do not come with a symbol. Specifically, if you inspect NMOSFET.lib, you'll see that none of the components includes a proper symbol. This means that all of them are supposed to use the default symbol provided at the beginning of the '.lib' file. As far as the component preview is concerned, the component symbol (if exists) is loaded at https://github.com/Qucs/qucs/blob/master/qucs/qucs-lib/qucslib.cpp#L508, so whenever a component is selected (either using the searchbox or not), there is an attempt to load its symbol. Unfortunately, the default symbol is loaded only when the library is selected using the combobox https://github.com/Qucs/qucs/blob/master/qucs/qucs-lib/qucslib.cpp#L351 This means whenever the user searches a part on the searchbox, the default symbol is not updated and QucsLib displays a wrong symbol. This is what I tried to solve using LoadDefaultSymbol().

About #553: In order to place a component into a schematic, QucsLib copies the part name and the corresponding library to clipboard https://github.com/Qucs/qucs/blob/master/qucs/qucs-lib/qucslib.cpp#L282 The problem below #553 is closely related to #550. Here, https://github.com/Qucs/qucs/blob/master/qucs/qucs-lib/qucslib.cpp#L514, if the component has not a symbol, content is empty, so QucsLib will use the default symbol. However, as said before, the default symbol is not reloaded after shortlisting, so 'DefaultSymbol' will be empty if the user didn't select the proper library in the combobox. In this sense, the program will skip lines 514 and 516, and, consequently, the selected part won't be the one the user picked from the list.

I've realized that the lines I added to solve #553 were redundant, so I removed them. 1ab5488#diff-17baae677f87ab5524a2a3f1f67ba886L517

(*) I understand symbol as a collection of lines, arcs, etc. which describe how the component will be represented on a schematic. Am I right?

[felix-salfelder]

your fix is okay for now, it's just in the wrong place, it's a problem with the symbol, not with qucslib, hence only the symbol should be fixed.

placed its code where this function was called.

the code does not belong to the qucs library. instead you should move it to the symbol and call Symbol->do_something() from where you put your code. preferrably it should be present only in the symbols where you need it. my understanding is, that not all symbols are affected.

why this makes sense: once we will have proper symbols, we don't want these hacks in the library, but only drop/replace the aching "symbols".

I understand symbol as a collection of lines, arcs, etc

what a symbol currently is, is a bit obscure, that might be one of the reasons for the permanent breakage. what it should be is definitely something that can represent a component. currently the "COMPONENT" class is closest, but it's not possible to switch components without hacks. a symbol also has a (exactly one) graphical representation.

if the component has not a symbol

this is a bug (read: should be impossible, should be fixed etc.), a component must be referenced from a symbol.

[andresmmera]

As suggested, I moved that code to SymbolWidget class. 2c08997#diff-e3112e94b1de131957494977de69e627R363

I agree with you, it seems clearer... this way Symbol->setSymbol( QString& SymbolString, const QString& Lib_, const QString& Comp_)) tries to find the default symbol when SymbolString is empty.

[felix-salfelder]

thanks. this looks better. but Symbol does not seem to be a symbol at all. great...

now the code is in int SymbolWidget::setSymbol. and it almost looks like it's intended like this. :(

before i merge this, lets check whether there is a sane way.

would it be possible to pass a reference to a COMPONENT instance to that widget? currently, you pass
const QString& Lib_, const QString& Comp_ which supposedly identifies a COMPONENT, right? where is this component? if we had it, we could do Somewidget::drawIt(my_component), which is exactly what we want...

[andresmmera]

Well, let me check if I am understanding correctly before start coding

In my opinion, if a well-defined relationship between component and symbol is needed, QucsLib should have the following changes:

i) Define a 'Component' class with the following fields:

a) string: Description
b) string: Component name (say IRF***, whatever)
c) string: Library (NMOSFET.lib,...)
d) Struct: Model
e) Class: Symbol

where struct Model should have the following fields:

 Struct Model
 {
    string Netlist;
    string VHDLModel;
    string VerilogModel;
 }

class 'Symbol' should contain a collection of geometric shapes and the functions required for displaying it.

That way, QucsLib will have a (private) 'Component' instance for keeping the reference to the last component the user selected from the list.

Am I on the right path?

[felix-salfelder]

Well, let me check if I am understanding correctly before start coding

perfect. so i am no longer the only one. but please don't start coding yet :).

what "should" be there is easy to tell. there should be a SYMBOL class, and each symbol should be an instance of a SYMBOL. not only in random places in the code but everywhere. example: if I load a library, then it gives me a set of symbols. period. anything else is implemented within these symbols.

the hard part is the transition. what you need is not so much a new class, but something that is closest to SYMBOL in the current implementation. imo that's clearly COMPONENT.

the transition could be this: make the COMPONENTS behave like symbols, then add a base class SYMBOL under COMPONENT with a clean interface (similar to what you suggest, but different, really). then turn COMPONENTS into SYMBOLS one by one. or just leave them the way they are, even if they are no longer needed.

i don't know how much should go into this PR, but it should be compatible.... what about my suggestion to pass a COMPONENT (future SYMBOL) to the widget, just for the drawing? that's something we want to have at some point...

[andresmmera]

Ok, I think I got the idea.

I made this fix with the premise of making minimal changes, just to solve #550 and #553.

As far as QucsLib is concerned, it doesn't exist a class 'COMPONENT' so it is not possible to pass a 'COMPONENT' object to SymbolWidget class to set the symbol. As you said before, the symbol is drawn using an identifier (Name+library) of the part the user selected. I mean, QucsLib works the following way:

i) The user picks a part from the list.
ii) QucsLib::slotShowComponent() is triggered.
iii) slotShowComponent() reads the part number and the library, and with these data, it calls SymbolWidget::SetSymbol(...) to build the symbol (i.e. graphical representation) and the dialog data is updated (symbol and description).
iv) When user decides to place the part into a schematic, it copies into clipboard something like this:

<Qucs Schematic 0.0.19>

<Lib T 1 0 0 8 -26 0 0 "NMOSFETs" 0 "IRF510" 0>

so, I guess that, for the moment, it is not worthy to define a new 'COMPONENT' class in QucsLib just to draw a symbol.

[felix-salfelder]

i don't see why qucslib and qucs should use completely different concepts of components really, it's one and the same thing on both ends.

what about just #include component.h". (we should move that file to qucslib, but only after we have symbols.h on the gui-end).

then we could go forward and do something similar to

COMPONENT s = make_the_component_this_is_all_about(stringmadness); // cleanup later
widget->draw(s);

i mean the code that implements the function i invented here does exist, and this is the place where we need it... i see your point about "minimal changes". but: duplicating code that essentially does the wrong thing in the wrong place, is not a good idea, even "for the moment". why: because this is exactly how the code has been degrading for years... :|

in the meantime, we should sketch a symbols baseclass and get it into the develop branch. it is important to see the big picture here, i guess. i only see 95% of it right now.

[andresmmera]

I've been taking a look to the source code (GUI). Probably, it would be easier to write a new (minimalistic) 'Component' class in QucsLib rather than just moving "component.h". I mean, "component.h" depends on "element.h" and includes a number of methods and variables not useful for QucsLib, such as copyComponent(Component*) or Schematic* containingSchematic

Don't take me wrong, I see your point and I think it leads to a better structured, and, hopefully, more readable code.

it is important to see the big picture here, i guess. i only see 95% of it right now.

Well, I only see 1% of it, so I need to dive deeper into the source code :)

[felix-salfelder]

i've just had a similar idea.

lets introduce symbol.h holding a SYMBOL class. this is the future base class for COMPONENT. it lives on the GUI end. we shall not touch COMPONENT yet. this must be done with other PRs (help is welcome!)

on the qucslib end, we introduce a LIB_COMPONENT class, that class inherits from SYMBOL. this class is your "minimalistic" component class, but which we will be able to migrate all the symbols related code to. and it will be very clear that qucslib actually is supposed to create SYMBOL instances rather than fishy string combinations. other library packages, such as qucs-geda-symbols will also generate SYMBOL instances...

why: this way, we can introduce a SYMBOL class, where it belongs and we can even do a partial test-drive... what do you think?

[andresmmera]

This way makes more sense to me. In my opinion, the harder part (by far) is the GUI end. To be honest, at this point, I am not capable to sense the implications of this changes in the GUI.

The GUI code seems to be quite entagled (although, maybe it's just me). So, until I have 100% clear how the components/symbols/whatever are treated on the GUI side, I won't be able to help you at this point. It's a question of time, I guess

[felix-salfelder]

I mean, "component.h" depends on "element.h" and includes a number of methods and variables not useful for QucsLib, such as copyComponent(Component_) or Schematic_ containingSchematic

cleanning this up is nontrivial. try to review #383... this (and more) is needed before switching to a SYMBOL class...

[felix-salfelder]

I am not capable to sense the implications of this changes in the GUI.

there should be none. the next 200 commits (related to this) will only move code into the right spot.

[felix-salfelder]

i have come this far: felix-salfelder@c15a7a1. feel free to either cherry-pick/complete it. or just wait. maybe more time tomorrow.

[in3otd]

I was thinking it would be nice to have the target issues fixed in 0.0.19; I quickly read the comments here and in felix-salfelder/qucs@c15a7a1 but I wasn't able to understand the status of the code. Almost finished, half-done, or?

IIUC, the first commits solved the issue but the solution was not future-proof; will it make sense to merge them in release-0.0.19 and the rest (if ready, of course) in develop?

[andresmmera]

In my opinion, the first commits work just to solve the issue, so I reckon that it could be merged for now. However, I would like @felix-salfelder to decide about if this PR should be merged or not.

To be honest, I haven't work on this topic since August... I'm focusing on Qucs-Filter :-)

[felix-salfelder]

yes, it's almost finished. i will try to catch up, please merge the hot-fix version for the next release candidate, if you wish.

(we should have more release candidates anyway...)

[guitorri]

Ok. I will cherry pick the two first commits for a hot-fix. I will let you rename or change this PR as you see fit.

On the risk of going off-topic: I understand you are trying to generalize things in the SYMBOL class. However I believe we would benefit most with library/cell/view infrastructure, which is the default in major proprietary EDA tools. A symbol is one of the possible views of a cell. Other views could be a piece of code (spice, verilog), a layout representation, a simulation setup, etc. A cell can be a component, a (sub-)circuit. Libraries, as you might expect, are collections of cells.

[felix-salfelder]

thanks.

A symbol is one of the possible views of a cell.

that probably makes sense. currently, i am trying to rearrange the existing code, so different sorts of symbols can be used at all (with the only implementation of SYMBOL being the currently available COMPONENTs).

currently i only want symbols that can represent custom models implemented in verilog particularly. but in the end it will be easily possible to also implement symbols that point to a "CELL", whatever that is going to be. note that SYMBOL only defines the interface to a graphical thing which represents a component in a netlist, the "backend" will always have to be implemented in a class that inherits from SYMBOL.

i am sure there will be choices to be made, somehow. are there any particular requirements for what you would like to do?

[guitorri]

I think the issues are fixed. As I understand you have another PR with with a refactor of qucs-lib.
Can I close this one?

[andresmmera]

This PR was simply a hot fix. I also think this can be closed