fritzing / fritzing-app

Fritzing desktop application
http://fritzing.org
Other
3.97k stars 823 forks source link

upgrade to qt 5 #2435

Closed davidperrenoud closed 10 years ago

davidperrenoud commented 10 years ago

From irasc...@gmail.com on February 21, 2013 08:46:06

What steps will reproduce the problem? 1. 2. 3. Attach your sketch file and/or custom part files to the bug report. If the sketch uses custom parts, save the sketch as shareable (under the file menu). What is the expected output? What do you see instead? What version of Fritzing are you using? On what operating system? Please use labels and text to provide additional information.

Original issue: http://code.google.com/p/fritzing/issues/detail?id=2437

davidperrenoud commented 10 years ago

From david.pe...@gmail.com on February 01, 2014 01:15:39

Here is my patch: https://github.com/davidperrenoud/fritzing/commit/94fcd0baa431906906163949b057764e82b5e722.patch You can read it in a more pleasant way in its GitHub original format: https://github.com/davidperrenoud/fritzing/commit/94fcd0baa431906906163949b057764e82b5e722 You have to install QHttp as described here: http://qt-project.org/forums/viewthread/24466 (don't forget to run syncqt.pl before running qmake)

davidperrenoud commented 10 years ago

From david.pe...@gmail.com on February 01, 2014 07:00:21

Regressions:

davidperrenoud commented 10 years ago

From david.pe...@gmail.com on February 01, 2014 10:16:34

The most common crash happens when saving a sketch just after having moved a part with the mouse (it doesn't crash if the part is moved with arrow keys). I think something is wrong in FileProgressDialog.

davidperrenoud commented 10 years ago

From david.pe...@gmail.com on February 02, 2014 04:57:31

This crash seems to be related to the deletion of FileProgressDialog and calls of processEvents(). Maybe this page can help, but not sure: http://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0

davidperrenoud commented 10 years ago

From irasc...@gmail.com on February 05, 2014 00:46:15

Thanks for your continued research. I had anticipated that there would be a period of testing required before the upgrade to qt5 would be ready. It's great that you have already identified some of the issues.

davidperrenoud commented 10 years ago

From andre.knoerig@gmail.com on February 22, 2014 03:28:30

Hi David, thanks for this nice first step! Would you be willing to continue this investigation? We can give you access to the repo and setup a branch.

Labels: -Priority-Medium Priority-Critical

davidperrenoud commented 10 years ago

From petryk.s...@gmail.com on February 23, 2014 09:28:52

I'm experiencing the same regressions when using the patch, but it's loads better than dealing with the insane amount of latency present in the current version.

davidperrenoud commented 10 years ago

From david.pe...@gmail.com on February 25, 2014 13:54:17

@André Yes, I will continue this investigation every now and then. To help me, could you ask Jonathan what the use of ProcessEventBlocker is? (if he's still working at IXDS)

davidperrenoud commented 10 years ago

From andre.knoerig@gmail.com on February 26, 2014 00:51:27

Awesome, should I set up a git branch for that? I'll see if Jonathan has an idea, he's on holiday at the moment though.

davidperrenoud commented 10 years ago

From andre.knoerig@gmail.com on February 26, 2014 01:58:00

Comment from Jonathan: "I think it had a larger role at one time; at the moment it seems limited to making sure that autosave isn't called during lengthy processes, such as autorouting. For the most ProcessEventBlocker::processEvents() calls QApplication::processEvents(), which is used to make sure that certain events get dealt with (like screen updates), when control is passed to something slow (like opening a sketch file)."

davidperrenoud commented 10 years ago

From rohan.ll...@gmail.com on April 26, 2014 15:31:15

I have also had a go at getting it to build on Qt5.

I have a git branch that builds/runs with both Qt4 and Qt5 https://bitbucket.org/rohanl/fritzing/branch/qt5 I don't see any crashes like you mention above.

However, I have a theory as to what is causing them

QtMsgHandler (qt4) and QtMessageHandler (qt5) have different signatures ( an extra context arg, and msg is QString instead of char *)

so the fMessageHandler method needs to be changed, otherwise at runtime you get a mismatch between args passed and expected.

davidperrenoud commented 10 years ago

From rohan.ll...@gmail.com on April 26, 2014 15:40:49

A couple of things I should mention:

I took the easy way out and just conditionally excluded all the version check code for now to remove the dependency on QHttp

I have built/run on Mac only. So if there's some issue with FileProgressDialog on lInux, I wouldn't have seen it

davidperrenoud commented 10 years ago

From david.pe...@gmail.com on April 27, 2014 03:20:33

Indeed, FileProgressDialog works fine with your version! (at least on Mac)

When I said "crashes like hell", I meant that I tried to use Fritzing Qt5 for production work and had to revert to Fritzing 0.8.7b to be productive. For example, both our versions sometimes crash when moving a part or right-clicking it. And instead of aligning on the grid on x=0 and x=1, parts align on x=0.5, x=1.5.

Also, your version seems to lose connections when moving parts in schematic view. I will try to compare our versions and send you a pull request.

davidperrenoud commented 10 years ago

From rohan.ll...@gmail.com on April 27, 2014 15:30:08

I'm just a very beginner hobbyist with an itch to scratch... Given a choice between learning how to use Eagle, or something else, I liked the idea of Fritzing, and thought I could contribute.

So, I don't have any "production" work to try out. Are there any large non trivial sketches available on the web somewhere to use for more realistic testing?

Now that I've looked at it a bit more closely:

I haven't done a detailed diff of our two versions, but from a quick look at the diffs, the changes seem to be restricted to:

There doesn't seem to be that much different between the 2.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 06, 2014 00:48:44

Hi Rohan, Hi David.

Fritzing is hiring me back for a few days to look into the Qt 5 upgrade, along with a couple of high-priority bugs. I am tempted to start with Rohan's branch, since I like the idea of being able to build with either Qt 4 or Qt 5. I have been reading the comments above, but wondered whether you have any additional advice or suggestions.

Thanks.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 07, 2014 06:07:30

I've added a "qt5" branch to the googlecode repository, which starts by pulling from Rohan's qt5 branch, then replaces QHttp with QNetworkAccessManager (and removes the conditional compilation flag NO_VERSION_CHECK).

So far I have only done the build under Linux 12.04/64 bit. In terms of the issues above:

davidperrenoud commented 10 years ago

From david.pe...@gmail.com on May 07, 2014 06:29:26

When I have time, I will try to make a video of all these bugs, log debugging outputs and post a test file. I can already say that it tested it on Mac with Qt 5.2.1. The Unicode problem ("♀ )gfnbmf*") could be seen on Generic female headers.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 07, 2014 06:41:24

The preferences languaage menu is set up in a file called "translatorlistmodel.cpp". Somehow the source is treated differently between version 4.8 and 5.2.1, so that some of the characters are not found (UTF-8 vs. Latin1?). It may be that the translation files need to be regenerated, or those strings have to be translated (in the source) to UTF-16 like some of the other language names.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 07, 2014 07:27:44

@comment 20: I am still not clear: is the funky text a part label for a particular generic female header? What is the text you were expecting? Thanks.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 07, 2014 10:57:01

Andre reported that align-to-grid is working fine on his mac build of the qt5 branch, so I took another look. Oops! align-to-grid is working fine on my linux build as well: what aligns to the grid is not a corner of the selection area of the part--the alignment uses the connection point--the center--of one of the connectors on the part (usually connector 0, but usually all the connectors are aligned anyway).

I am tempted to cross this off the list of problems: please let me know whether you are still getting buggy alignment behavior.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 07, 2014 11:00:43

QTimer works differently in Qt 5--the default timer behavior is less accurate than in Qt 4. I don't think any QTimer instances in Fritzing require the Qt4 level of accuracy, but it needs to be looked into.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 07, 2014 11:39:32

r416e94a992dc : made a few timers 'precise', leaving the rest to default. Could probably make a few of them 'coarse', but that's for a rainy day.

davidperrenoud commented 10 years ago

From rohan.ll...@gmail.com on May 07, 2014 16:01:44

I have only built/run on Mac (Mavericks) here are my findings

  • crashes like hell -- unable to replicate so far under linux. I have tested with one of the larger example files, but this still does not reach "production work" scale.

I too, couldn't reproduce, but have only tested with "trivial" sketches

  • can't use backspace or delete key to delete a part -- unable to replicate under linux

This is a problem with Qt5 on Mac. I have a fix for this. I haven't pushed it to my branch yet as my work area has a bunch of other things I've been experimenting with, but here is a diff. Note: I also took the opportunity to add a new Accelerator on Mac Option-Delete for "Delete Minus"

diff --git a/fritzing/src/mainwindow/mainwindow.cpp b/fritzing/src/mainwindow/mainwindow.cpp index ba00490..78d5bbe 100644 --- a/fritzing/src/mainwindow/mainwindow.cpp +++ b/fritzing/src/mainwindow/mainwindow.cpp @@ -1340,6 +1340,28 @@ bool MainWindow::eventFilter(QObject object, QEvent event) { ProcessEventBlocker::processEvents(); }

+#if defined(Q_OS_MAC) && (QT_VERSION >= 0x050000)

diff --git a/fritzing/src/mainwindow/mainwindow_menu.cpp b/fritzing/src/mainwindow/mainwindow_menu.cpp index 3b7c4d6..d1fe9b1 100644 --- a/fritzing/src/mainwindow/mainwindow_menu.cpp +++ b/fritzing/src/mainwindow/mainwindow_menu.cpp @@ -897,15 +897,18 @@ void MainWindow::createEditMenuActions() { m_deleteAct = new QAction(tr("&Delete"), this); m_deleteAct->setStatusTip(tr("Delete selection")); connect(m_deleteAct, SIGNAL(triggered()), this, SLOT(doDelete()));

  • The most common crash happens when saving a sketch just after having moved a part with the mouse -- unable to replicate under linux

I haven't seen - admittedly with very simple sketches

  • crash when moving a part or right-clicking it -- unable to replicate under linux

I haven't seen - admittedly with very simple sketches

  • instead of aligning on the grid on x=0 and x=1, parts align on x=0.5, x=1.5. -- Seems to happen in all 3 views, but the x and y offset is different. This is also true of the 4.8.6 build of the same source.

I was confused by this too. But once I understood that it aligns on the connectors, it looks ok

  • seems to lose connections when moving parts in schematic view. -- unable to replicate under linux

I haven't seen - admittedly with very simple sketches

  • When I change views, the Views menu ends up with multiple items "checked" with a dot -- unable to replicate under linux

Happens on Mac. See comment before about a patch to use native checked items anyway

  • Non ascii text gets corrupted (eg preferences->language) -- I see it under preferences->language, but not with the 4.8 build of the same source. I don't see the degree-symbol mangling.

This seems to behave differently under Qt5. String literals in the source are not in UTF-8. eg:

In Français, the source contains 0xE7, whereas the correct UTF-8 sequence is 0xC3 0xA7 (octal: 303 247)

This could be fixed by explicitly using utf8 in the few cases it is needed like above. However, I think the "proper" fix is to fully embrace utf8 and use that encoding everywhere: http://www.macieira.org/blog/2012/05/source-code-must-be-utf-8-and-qstring-wants-it/ For qt4 compatability, this probably means calling:

QTextCodec::setCodecForCStrings(QTextCodec::codecForName("UTF-8")) QTextCodec::codecForTr(QTextCodec::codecForName("UTF-8"))

davidperrenoud commented 10 years ago

From rohan.ll...@gmail.com on May 07, 2014 16:13:19

Just one comment on the crashes... I did see one reproducible crash.

I accidentally did this one, and found I could reproduce a crash. It crashes because the view is NULL somewhere (sorry this is from memory, don't have a stack trace handy) in code that is called from a timer

I think what's happening is that the first click is selecting the number of pins, then the second click is changing the selection.

Once the timer fires, it is using "global" state that has changed since the time the action was done.

If Qt5 timers are not as accurate, this could explain why it is happening more often.

I don't quite understand the need for a timer, I;m guessing it's to push the processing off the GUI thread.

I think either:

Anyway, as I said I really don't understand the threading model, so ignore if this doesn't make sense.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 08, 2014 22:19:32

@comments 26 & 27: thanks Rohan, this is very helpful. I will add your patch for the delete key this weekend, and also the menu checkbox patch. Since my time is limited, I converted the preferences language names to UTF-16. I will also look into the QTimer crash, though I won't have time to generally re-architect that behavior as you suggest.

davidperrenoud commented 10 years ago

From irasc...@gmail.com on May 09, 2014 13:11:04

r549ddcc7ff28 and previous: Rohan's delete-key patch is in; did a simpler fix for the checkmarks. Was able to replicate the timer crash once in about 10 tries, but didn't catch the culprit; will have to try again.

davidperrenoud commented 10 years ago

From andre.knoerig@gmail.com on July 17, 2014 07:40:05

Closing this, since we now released this version. Thanks to all who helped! Please add individual issues for new bugs.

Status: Fixed