Webengine JavaScript refactor #403
Conversation
src/ui/include/mainwindow.h
Outdated
| @@ -256,16 +254,13 @@ private slots: | |||
| int saveAs(EditorTabWidget *tabWidget, int tab, bool copy); | |||
| QUrl getSaveDialogDefaultFileName(EditorTabWidget *tabWidget, int tab); | |||
| void setupLanguagesMenu(); | |||
| void transformSelectedText(std::function<QString (const QString &)> func); | |||
| void transformSelectedText(QString (*func)(const QString&)); | |||
There was a problem hiding this comment.
Why that change? Isn't a std::function all around more convenient and more compatible?
There was a problem hiding this comment.
It didn't make sense to have the overhead for something that doesn't need any captures to begin with.
|
Is this PR supposed to also have all the changes from #402 in it? |
|
Here's a question for my understanding (because I only take a glance at some of the changes at it's hard to keep up): Right now we've two ways of getting data: the async calls you're implementing right now and using signals to set |
|
That works great for data that's accessed rather passively... but not so much for on-demand requests. As an example, if we got 10 functions that use getSelectionsText... that's either 10 signals + lambdas. or one signal... a bunch of functors and a message transport handler. ... Though the idea of just sending a "request" and intercepting the reply does sound nice, lol.... it's basically what we're doing with the async setup.... just.. uglier? |
|
The biggest blocker for that change is "getValue"... for the record... the way its tied into sessions, mainwindow, docengine, etc... makes it a pain in the behind to get working right. |
|
I could change all the get[GetFunctionName] thingies to request... and provide a small flexible API for handling callbacks there in one place. It's probably doable now that I think about it. So I'm probably gonna try... because segmented API annoys the crap out of me. |
|
I can't really think of a way to implement that for the others without blocking the current running thread, lol... Asynchronous programming is really tedious. |
|
Here's an example of what the code would look like with signals/slots... assuming you did an in-place lambda. void frmSearchReplace::on_searchStringEdited(const QString &/*text*/)
{
NqqSettings& s = NqqSettings::getInstance();
if (s.Search.getSearchAsIType()) {
if (ui->actionFind->isChecked()) {
Editor *editor = currentEditor();
auto conn = std::make_shared<QMetaObject::Connection>();
*conn = connect(editor, &Editor::selectionReply, [this, conn, editor] (QList<Editor::Selection> selections) {
if (selections.length() > 0) {
editor->setCursorPosition(
std::min(selections[0].from, selections[0].to));
}
findFromUI(true);
QObject::disconnect(*conn);
});
editor->requestSelections();
}
}
}
|
I think you misunderstood. What I mean is that, right now, Why is it we don't do that for stuff like Sorry if that's just me being daft :P
Kill it with fire! |
|
Lol... oh I get you. I think @danieleds said something about not
storing values that aren't frequently accessed or something along those
lines... though I don't think it would be an issue with "selections" as
it's just an integer. I think he was mainly refering to getValue,
getSelectionsText, etc....
|
|
@JuBan1, how's this look for getSelections? :)
|
|
I think it's good- if it works :p The less we have to worry about async operations in all of the code using |
|
Seems to work fine... getSelections is only used one place in the code
so it wasn't hard to test. :P... Speaking of which I just figured out
how to iterate over a QVariant without performing conversions... this
has been a real annoyance for me for a while, lol.
|
|
So the proxy is probably going to go back to just sending messages/signals... as it stands it doesn't really need to store any values, because that's being handled by Editor as the data passes through. So long as I tread carefully I should be able to implement a very clean interface for all of this and reduce the need to rely on javascript callbacks in general. |
|
EVIL! Unwinding that javascript spaghetti took forever! There might be a couple of functions in the wrong class... but aside from that, I think its a bit cleaner.... or at least more reliable. This changeset really got out of hand, lol... |
|
@JuBan1, I'm going to go out on a limb and assume @danieleds is having a few of his busy weekdays. Care to go over the Javascript portion and see if there's any code I could move around and/or more correct class name/filename for certain things? |
src/editor/classes/Helpers.js
Outdated
|
|
||
| // Since the original tab is replaced by a space we only need numSpaces-1 | ||
| // new spaces. | ||
| tabToSpaceCounter += numSpaces-1; |
| C_CMD_TAB_TO_SPACE(data) | ||
| { | ||
| editLines(function (x) { | ||
| tabToSpaceCounter = 0 |
There was a problem hiding this comment.
Does this still work since tabToSpaceCounter is now in App.helpers?
There was a problem hiding this comment.
This comment is no longer relevant since I moved helpers to a plain functions file. It all works as-is
src/editor/classes/MessageHandler.js
Outdated
|
|
||
| return x.replace(/ +/g, function(match, offset) { | ||
| return App.helpers.spaceToTab(match, offset, tabSz) | ||
| }); |
There was a problem hiding this comment.
While I like the idea of moving code logic into another class, I think this is a bad place to do it. There are really tricky interactions here where the algorithm won't work if a user forgets to call App.helpers.spaceToTabCounter = 0 or similar.
Previously all code was right next to each other so the interactions were easy to see and follow but now that code is more encapsulated it becomes very hard to follow code execution.
Can we move all the code to App.helpers so we only cal something along the lines of editLines(App.helpers.spaceToTab) or similar?
Same goes for the other XtoY functions.
There was a problem hiding this comment.
How about we forget about "Helpers" all together and make it a plain functions file since they're all utility functions... this would fix all three problems I think.
There was a problem hiding this comment.
My main point was that tightly coupled code should either be near each other or should be very clear to reduce code spaghetti. So if you think that solves it, give it a try.
src/editor/classes/CodeMirrorInit.js
Outdated
| else | ||
| cm.execCommand("insertSoftTab"); | ||
| }, | ||
| "Shift-Tab": function (cm) { |
There was a problem hiding this comment.
So, button presses are pretty much events too. Might be a nice idea to move these two functions into EditorEventHook.js as well?
|
Take my input with a grain of salt. My JavaScript is not great :) |
|
I'm trying to think of the best way to go about those helper functions in particular... Having them in a class does break code logic a bit. I could write a shortcut "functions" file that calls the appropriate App.helpers.Function... but in all reality I think that's just nesting, lol. |
|
I seriously need to learn how to base a branch on a branch using git... merge conflicts are evil, lol. |
|
a.k.a. I didn't mean to include #402's changeset into this. |
Add minor cache mechanism to avoid reparsing file.
|
@danieleds I already replied to that comment you tagged me in... went a different route with that so it should be fine there. I'm just ready to get this merged so I can move on to the next painful thing I have to smack around to get WebEngine working. -.- |
src/ui/include/EditorNS/editor.h
Outdated
| static void invalidateEditorBuffer(); | ||
|
|
||
| //FIXME: Possibly un-necessary? | ||
| /* | ||
| struct LanguageGreater { |
There was a problem hiding this comment.
Do they actually need to be sorted? From what I can tell QMap is sorting everything anyways... exactly like the sorting function was doing, lol.
There was a problem hiding this comment.
Perfect! If that's the case, then we can remove it.
|
Good to know! :D Okay then, let's also forget about that stuff in CommunicationsManager for now. |
|
Hmm, segfault on startup at languagecache.cpp#94... I need to investigate. |
|
@danieleds, that's new to me... I tested the cache heavily without any oddities. |
|
Care to share the full gdb output on that? |
|
|
Quite interestingly, it doesn't happen if you replace the iterator with this (optimizable) code: for (QString key : json.object().keys()) {
QJsonObject mode = json.object().value(key).toObject();
Language newMode;
newMode.id = key;
newMode.name = mode.value("name").toString();
newMode.mime = mode.value("mime").toString();
newMode.mode = mode.value("mode").toString();
newMode.fileNames = mode.value("fileNames")
.toVariant().toStringList();
newMode.fileExtensions = mode.value("fileExtensions")
.toVariant().toStringList();
newMode.firstNonBlankLine = mode.value("firstNonBlankLine")
.toVariant().toStringList();
m_languages.append(std::move(newMode));
} |
|
I'm gonna guess it's a bug in the iterator for QT 5.7... which isn't a big deal, but it's annoying, nonetheless.... one sec and I'll modify to use your code. |
|
I believe that too. Qt bugs are starting to get annoying. |
|
Try this commit and see if it fixes it for you. |
|
There's a lot of black magic under the hood of QT that I'm not very fond of... It doesn't bother me to write a little extra code to make something work the way I want it... I don't need QT implying "optimizations" for me. I started programming before we had fancy smart pointers and such, after all. :P Oh the days of malloc() and free() were simpler times. |
|
Yup, working. |
|
Is it? It might not increase performance in that case... but that "eval once" optimization might not apply to every compiler under the sun, so it doesn't hurt anything at least. |
|
I think it actually slows it down a bit with it as a variable (odd)... removed it. :) |
|
Actually, the semantic would be completely different in that case. I think that behavior must be well defined somewhere, as it's not a decision that should be left to the compiler implementation. Actually, you can find it in the official specification, between page 141 and 142: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf (just nitpicking :P) |
|
The performance is very decent, language initialization is roughly 1/100th of a second(on my hardware) on average... and lookups are on a scale I can't really measure accurately, as they should be. The only way to make it faster at this point would be to write a JSON parser from scratch and read it directly... but I'm not that hellbent on juicing something so simple, lol. |
|
Also, initialization doesn't happen until the first call of getInstance(), which keeps it self-contained, much like nqqsettings |
|
Yeah it's not a performance-critical portion of code. I was pointing it out just because of your old comment above that line, which was fine for the old loop but now it wasn't true anymore :P Can you delete that commented stuff which was used to sort languages? |
|
Done. |
|
Before we close this PR... should probably touch on what else needs to be done to complete QWebEngine... we're getting reasonably close, I think. I'm just not sure if there are anymore barriers that we're unaware of as of yet. |
|
I still have problems with performance. I guess that now is a good time for little fixes (I saw some glitches somewhere) and smart performance optimizations. Other than that, it seems that the difficult part is over. |
So.... this PR looks a lot fatter than it really is... it partially implements the json language file bit we were talking about.
At some point or another... I accidentally fixed that notorious preview gutter bug while I was working on this as well.
There's still some work to be done... but the overall performance of this is very good! This also happens to get rid of the QJSEngine requirement from the initial QWebEngine PR.
I'll most likely start cleaning up the javascript in this PR as well. Its a mess right now and could really use some TLC. Feel free to review the code when you guys get a chance. It's still a work in progress though, lol.