Qucs / qucs

Qucs Project official mirror
http://qucs.sourceforge.net/
GNU General Public License v2.0
1.16k stars 213 forks source link

Fix issue #892 #895

Closed andresmmera closed 5 years ago

andresmmera commented 5 years ago

See https://github.com/Qucs/qucs/issues/892

guitorri commented 5 years ago

Can you put changes in formatting a separate commit? Right now it is hard to find and review the actual change/fix.

andresmmera commented 5 years ago

Ops, I installed a clang plugin for QtCreator. It aplies the clang format automatically after saving... No worries, I'll separate that tomorrow.

andresmmera commented 5 years ago

I added an extra argument to num2str() to handle unit. In my opinion, that seems more correct. ...It is simpler to keep num2str() untouched and simply add the meter units "m" in those lines where a TLIN is added...

I'm gonna revert these commits...

in3otd commented 5 years ago

I'm gonna revert these commits...

just restart from the base branch and force-push, it's less work :grin:

andresmmera commented 5 years ago

Yes, I was doing that :-)

andresmmera commented 5 years ago

@in3otd

And while you are at it, please re-add the space between the number and the prefix

Since num2str() provides the suffix, it is not possible to do that. For example,

*s += QString("<TLIN Line1 1 %1 0 -41 46 0 1 \"%2Ohm\" 1 \"%3m\" 1 \"0 dB\" 0 \"26.85\" 0>\n").arg(x).arg(Filter->Impedance).arg(num2str(l[i-1]));

would return:

<TLIN Line1 1 x 0 -41 46 0 1 50Ohm 1 30m m 1 0 dB 0 26.85 0>\n">

Actually it was easy as pie. See c253d57

in3otd commented 5 years ago

(moving back the discussion to the PR, I commented on the issue by mistake)

And BTW, spaces between number and prefix/units should be used for all the units, not only ohm

Yes, that was done by adding the space at num2str()

ah, right, I missed that and the fact that impedances do not go thru num2str()

andresmmera commented 5 years ago

I've just removed the first two commits where we manually added the H, F and m suffixes when adding the components. Then, filter::num2str() was replaced by misc::num2str() and after that, I did some adjustments...

All the implementations were tested, but some extra testing is appreciated :-)