egm supp #138

fundies · 2020-07-25T23:31:44Z

enigma-dev/enigma-dev#2089

RobertBColton · 2020-08-05T16:13:40Z

MainWindow.cpp

+  if (!EnigmaRoot.filePath().isEmpty()) {
+    _event_data = std::make_unique<EventData>(ParseEventFile((EnigmaRoot.absolutePath() + "/events.ey").toStdString()));
+  } else {
+    qDebug() << "Error: Failed to locate ENIGMA sources. Loading internal events.ey.\n" << "Search Paths:\n" << MainWindow::EnigmaSearchPaths;
+    QFile internal_events(":/events.ey");
+    internal_events.open(QIODevice::ReadOnly | QFile::Text);
+    std::stringstream ss;
+    ss << internal_events.readAll().toStdString();
+    _event_data = std::make_unique<EventData>(ParseEventFile(ss));
+  }
+
+  egm = egm::EGM(_event_data.get());


A lot of this should probably go into its own project handling class, like Josh had me do for recent files. Jamming everything into the main window class is not a good, we want to design this with loose coupling of components, something that signals and slots help accomplish.

Sounds like a task for another day.

This is new code, all new code should be written correctly the first time. I did it correctly with recent files the first time.

This is just the first draft to make it work/build against enigma. As long as you hold up this pr over nonsense design choices the enigma CI remains broken. This pr was meant to be merged the same time as the enigma side one. You can refactor it later but it needs to go in now.

RobertBColton · 2020-08-05T16:15:58Z

MainWindow.cpp

@@ -155,7 +188,7 @@ MainWindow::MainWindow(QWidget *parent) : QMainWindow(parent), _ui(new Ui::MainW
  openNewProject();
 }

-MainWindow::~MainWindow() { delete _ui; }
+MainWindow::~MainWindow() { diagnosticTextEdit = nullptr; delete _ui; }


This isn't necessary it uses the Qt ownership mechanism (its parent is specified in the constructor). This is C++, not C, we're not concerned about zero'ing out our pointers. Remove this, it's not idiomatic Qt or C++ at all.

https://doc.qt.io/qt-5/objecttrees.html

The editor segfaults on close without this. You're referencing another pointer so it's never cleared.

I think it's the signals queueing messages, something else is wrong, this is only a bandaid. The correct fix is to disconnect the output/log signals so it stops trying to reference that after it's gone.

it also correct to null the pointer

RobertBColton · 2020-08-05T16:18:14Z

MainWindow.cpp

@@ -302,7 +337,7 @@ void MainWindow::openFile(QString fName) {

  MainWindow::setWindowTitle(fileInfo.fileName() + " - ENIGMA");
  _recentFiles->prependFile(fName);
-  openProject(std::unique_ptr<buffers::Project>(loadedProject));
+  openProject(std::move(loadedProject));


Should document somewhere, maybe in the header, that openProject takes ownership of the buffer.

RobertBColton · 2020-08-05T16:19:39Z

MainWindow.h


  void openSubWindow(buffers::TreeNode *item);
  void readSettings();
  void writeSettings();
  void setTabbedMode(bool enabled);
+  static QFileInfo getEnigmaRoot();


This is a horrible function name, also add comments to clarify what it does.

It gets the root of enigma :/ I dunno what else you'd name it.

ENIGMA root could be anything, is my point, it could be interpreted as meaning the protobuf/tree root or etc. Probably GetEnigmaFileSystemRoot would be miles better.

none of the things you listed are enigma

RobertBColton · 2020-08-05T16:20:04Z

MainWindow.h

  std::unique_ptr<buffers::Project> _project;
  QPointer<RecentFiles> _recentFiles;
+
+  std::unique_ptr<EventData> _event_data;
+  egm::EGM egm;


Move all of this stuff to the private section.

It is in private....

RobertBColton · 2020-08-05T16:22:06Z

images.qrc

@@ -100,5 +100,6 @@
        <file alias="banner.png">Images/banner.png</file>
        <file alias="icon.ico">Images/icon.ico</file>
        <file alias="transparent.png">Images/transparent.png</file>
+        <file alias="events.ey">Submodules/enigma-dev/events.ey</file>


We should rename images.qrc or create a separate resources.qrc because the user will later replace icon packs dynamically. We don't want them to have to put events.ey into their icon pack.

Right, I was going to rename this file but didn't care to do it here

I really wouldn't put it in with the images.
https://doc.qt.io/qt-5/resources.html#external-binary-resources

RobertBColton · 2020-08-05T16:23:32Z

MainWindow.cpp

+MainWindow::MainWindow(QWidget *parent) : QMainWindow(parent), _ui(new Ui::MainWindow), _event_data(nullptr), egm(nullptr) {
+
+  if (!EnigmaRoot.filePath().isEmpty()) {
+    _event_data = std::make_unique<EventData>(ParseEventFile((EnigmaRoot.absolutePath() + "/events.ey").toStdString()));


Ask @JoshDreamland why ParseEventFile doesn't just return a unique_ptr for me.

EventData should be an object not a pointer but because it doesn't have a default constructor I need to conditionally initialize it with different constructors by making it a pointer.

RobertBColton · 2020-08-05T17:01:56Z

RadialGM.pro

@@ -178,4 +178,5 @@ FORMS += \
 RESOURCES += \
    images.qrc

-DISTFILES +=
+DISTFILES += \
+  Submodules/enigma-dev/events.ey


I don't know if this was necessary, the documentation says it only works on Unix, and you've already put it in the resources file.
https://stackoverflow.com/a/38102991

qtcreator did this not me

Hrm idk then.

RobertBColton

Ok start but this needs cleaned up more.

fundies and others added 6 commits July 25, 2020 19:31

Update MainWindow.cpp

3242c3d

Update MainWindow.cpp

068280b

Update MainWindow.cpp

8c2d352

merge master

1ce695a

events backup

7ce6ee6

Update ServerPlugin.cpp

8ec485d

fundies requested a review from RobertBColton August 5, 2020 11:50

RobertBColton reviewed Aug 5, 2020

View reviewed changes

merge master

e8d7f8b

RobertBColton approved these changes Aug 7, 2020

View reviewed changes

RobertBColton merged commit 79a68a7 into master Aug 7, 2020

RobertBColton deleted the egm-supp branch August 7, 2020 23:31

RobertBColton mentioned this pull request Aug 12, 2020

Pro EGM Fixes #150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

egm supp #138

egm supp #138

fundies commented Jul 25, 2020 •

edited by RobertBColton

Loading

RobertBColton Aug 5, 2020

fundies Aug 5, 2020

RobertBColton Aug 5, 2020 •

edited

Loading

fundies Aug 7, 2020 •

edited

Loading

RobertBColton Aug 5, 2020 •

edited

Loading

fundies Aug 5, 2020

RobertBColton Aug 5, 2020

fundies Aug 5, 2020

RobertBColton Aug 5, 2020

RobertBColton Aug 5, 2020

fundies Aug 5, 2020

RobertBColton Aug 5, 2020

fundies Aug 5, 2020

RobertBColton Aug 5, 2020

fundies Aug 5, 2020

RobertBColton Aug 5, 2020 •

edited

Loading

fundies Aug 5, 2020 •

edited

Loading

RobertBColton Aug 5, 2020

RobertBColton Aug 5, 2020

fundies Aug 5, 2020

RobertBColton Aug 5, 2020

fundies Aug 5, 2020

RobertBColton Aug 7, 2020

RobertBColton left a comment

egm supp #138

egm supp #138

Conversation

fundies commented Jul 25, 2020 • edited by RobertBColton Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertBColton Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

fundies Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

RobertBColton Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertBColton Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

fundies Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertBColton left a comment

Choose a reason for hiding this comment

fundies commented Jul 25, 2020 •

edited by RobertBColton

Loading

RobertBColton Aug 5, 2020 •

edited

Loading

fundies Aug 7, 2020 •

edited

Loading

RobertBColton Aug 5, 2020 •

edited

Loading

RobertBColton Aug 5, 2020 •

edited

Loading

fundies Aug 5, 2020 •

edited

Loading