Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Component Properties dialog #1054

Merged
merged 43 commits into from
Nov 18, 2024
Merged

Conversation

iwbnwif
Copy link
Contributor

@iwbnwif iwbnwif commented Nov 10, 2024

The main aim of this change is to switch from editing equations by selecting them in a table and then typing in a separate field, to entering them using free text. At the moment the editing is very free(!) and it might be that it needs to be more constrained.

The main problem I started off trying to solve is when you accidentally overwrite an existing equation by clicking 'apply' instead of 'add'. I think that free text is an easier way of entering equations, especially longer lines that didn't fit completely in the edit box.

Some basic syntax highlighting is included for Ngspice keywords only. It would also be nice to detect some basic mistakes and underline them - but this is a future enhancement.

Any rows that do not include an equals '=' are ignored and will be lost. Maybe in future it would be nice to include comments?

This is quite a substantial change and although there are a few jobs still to do, I would welcome any feedback at this stage. I can only practically test the changes on Linux at the moment so bug reports from other platforms are welcome.

TODO:

  • Auto update sweep step / sweep points for log sweeps
  • Translated text?
  • Add special property names - i.e., for log sweeps (per decade instead of step)
  • Update components from SPICE file.
  • Implement highlighting.
  • Have "Export" as a check box, or option list for equations.
  • .INCLUDE components have multiple files
  • Check for memory leaks (will be done as the final step).

A second change is to edit component properties directly in the table rather than in a separate dialog. Again, I am sure there are some corner cases that I have missed.

I have constrained my changes to just two files. The rest of the simulation and drawing is unchanged.

iwbnwif and others added 28 commits November 2, 2024 18:39
… existing code will be removed after this commit.
…ted. Step and Points fields do not auto update. Open SPICE file not implemented.

qDebug() statements will be largely removed after this commit.
Set the property table values after openPersistentEditor to avoid them being selected by the editor.
…nctionality.

Add a BoundComboBox class and implement the .SW Sim property using it.

Implement basic functionality for sweep and parameter pages. A lot of existing code will be removed after this commit.

First minimum inviable state. File open and edit buttons not implemented. Step and Points fields do not auto update. Open SPICE file not implemented.
qDebug() statements will be largely removed after this commit.
Signed-off-by: Anton Midyukov <[email protected]>
Set the property table values after openPersistentEditor to avoid them being selected by the editor.
@ra3xdh ra3xdh marked this pull request as draft November 11, 2024 17:25
@ra3xdh
Copy link
Owner

ra3xdh commented Nov 11, 2024

The issues mentioned in #1052 seems to be fixed. I have marked this PR as WIP.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 16, 2024

Still have no effect with the latest commit. The hasFocus() returns false and currentRow() returns -1 The if block in the keyPressEvent handler is not executed.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 17, 2024

@iwbnwif , If nothing else helps, please integrate the following patch. It blocks the dialog accept when Return key is pressed. It seems to be a Wayland bug, because the dialog closes before the table contents is updated.

diff --git a/qucs/components/componentdialog.cpp b/qucs/components/componentdialog.cpp
index 51540806..0888262b 100644
--- a/qucs/components/componentdialog.cpp
+++ b/qucs/components/componentdialog.cpp
@@ -517,17 +517,11 @@ ComponentDialog::~ComponentDialog()
 // property table.
 void ComponentDialog::keyPressEvent(QKeyEvent* e)
 {
-  qDebug() << "Dialog key event" << e->key();
-
-  if (e->key() == Qt::Key_Return && propertyTable->hasFocus()) {
-    int row = propertyTable->currentRow();
-    if (row < propertyTable->rowCount())
-      propertyTable->setCurrentCell(row + 1, 1);
-    else
-      slotOKButton();
-  }
-  else
+  // Workaround for Qt+Wayland bug. Otherwise the Key_Retirn invokes slotOK()
+  // and table contnents not updated. Not possible to enter value.
+  if (e->key() != Qt::Key_Return) {
     QDialog::keyPressEvent(e);
+  }    
 }
 
 // -------------------------------------------------------------------------

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Nov 17, 2024

Thank you for the patch, I was thinking along those lines too. However, please can you test 14f603b as I would like to know if closing the persistent editor saves the changes.

If this works, I will clean up the code otherwise I will apply your patch.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 17, 2024

The 14f603b still have no effect. The Wayland issue may be more complex. I think we have to block the Return key processing as proposed before.

@dsm
Copy link
Collaborator

dsm commented Nov 17, 2024

I tested latest build on windows pressing enter don't change value too. But pressing other cell and then press value and value changed after enter or pressing tab same. So issue shouldn't be a not wayland specific.

value.webm

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 17, 2024

@dsm, Yes the KDE+Wayland has the same behavior as your screencast. I didn't check Windows.

@dsm
Copy link
Collaborator

dsm commented Nov 17, 2024

MacOS version don't affect this bug because pressing enter don't close panel, Btw Show section has editable text area if double pressing right after the check box icon. @ra3xdh I tested win11

@dsm
Copy link
Collaborator

dsm commented Nov 17, 2024

Also adding text right after check box and press enter don't close property panel.

value.webm

@dsm
Copy link
Collaborator

dsm commented Nov 17, 2024

Warning: QAbstractItemView::commitData called with an editor that does not belong to this view (:0, )

I tested Linux AppImage in WSL and I got this warning only pressing enter, pressing OK not produce this warning.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 17, 2024

@dsm , The true Linux version also prints the same warning message after pressing Enter in the table cell.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Nov 17, 2024

Warning: QAbstractItemView::commitData called with an editor that does not belong to this view (:0, )
I tested Linux AppImage in WSL and I got this warning only pressing enter, pressing OK not produce this warning.

Please let me know if this message is still shown after 174d206

I have implemented Qt::Return skipping as suggested by @ra3xdh and removed closing the QTableWidget persistent cell editor when pressing OK.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 17, 2024

Please let me know if this message is still shown after 174d206

The warning message has gone.

@dsm
Copy link
Collaborator

dsm commented Nov 17, 2024

I think, first select of table select return different state after tab select, issue is not related to pressing enter.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Nov 17, 2024

I think, first select of table select return different state after tab select, issue is not related to pressing enter.

Yes, I agree. I cannot reproduce the behavior that you are seeing but I do see something different on the first press, or first press after receiving focus.

@dsm
Copy link
Collaborator

dsm commented Nov 17, 2024

@ra3xdh KDE+X11 has the same issue in CachyOS thus issue is not wayland specific linux and windows affected only macos not affected because pressing enter didn't close dialog.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 18, 2024

Let's accept the solution for Enter key from the latest commits. @iwbnwif , Please cleanup the debug printing from the latest commits.

I have found an another bug with this PR. When using Fill from SPICE the values are not filled in the device and in the table. It should be visible in the table after user clicks OK in the dialog window. Here is the screencast.

Here is the source of the 1N4007 model. It worked fine with the old version:

*SRC=1N4007;DI_1N4007;Diodes;Si;  1.00kV  1.00A  3.00us   Diodes, Inc. diode
.MODEL DI_1N4007 D  ( IS=76.9p RS=42.0m BV=1.00k IBV=5.00u
+ CJO=26.5p  M=0.333 N=1.45 TT=4.32u )
Screencast_20241118_091413.webm

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 18, 2024

After adding updatePropertyTabel() at the componentdialog.cpp line 982 the Fill from SPICE works as expected.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Nov 18, 2024

After adding updatePropertyTabel() at the componentdialog.cpp line 982 the Fill from SPICE works as expected.

Oops! I hesitated about this because it means that even if the user presses 'Cancel' on the Component Dialog then the schematic component will still be updated. I was considering to pass a temporary component to fillFromSpiceDialog and then copy its contents to updatePropertyTable().

This would be more consistent with the way the rest of the dialog works but I wonder if there is a reason why the existing code works this way. This is not a feature that I have used so far so I am afraid I don't know much about it.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 18, 2024

it means that even if the user presses 'Cancel' on the Component Dialog then the schematic component will still be updated.

Probably I have missed this point when designing this feature. But anyway, let's revert the operation of the Fill from SPICE in this PR. After fixing this problem and cleaning the debug printing I will merge this. I didn't find any other major issues.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Nov 18, 2024

Hopefully de5fff9 will solve the SPICE model problem. It is a little bit hacky as I should probably create copy constructors for Component and/or Property but I don't want to touch too many files.

@ra3xdh
Copy link
Owner

ra3xdh commented Nov 18, 2024

Yes, now everything works as expected.

@ra3xdh ra3xdh merged commit dc6f657 into ra3xdh:current Nov 18, 2024
7 checks passed
@ra3xdh
Copy link
Owner

ra3xdh commented Nov 18, 2024

I have merged the PR. Stay connected, if some problems will be found by another users.

@iwbnwif
Copy link
Contributor Author

iwbnwif commented Nov 18, 2024

I have merged the PR. Stay connected, if some problems will be found by another users.

Yes, I am sure there will be a lot of small problems (hopefully no big ones). The dialog deals with a lot of special cases and I tried to simplify that a bit by having kind of look up tables but I wasn't entirely successful.

I don't have a Windows development platform but I already noticed that the table line editors don't resize when you adjust the column width on Windows. This also happens on Linux but is masked by the fact that borders are not drawn around the line edit.

Also, I would like to make the compound line edit / ... button take the whole space of the table cell. At the moment there is a border that I cannot manage to get rid of.

Hopefully, these are cosmetic issues that can be corrected in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants