Closed Bug 586722 Opened 10 years ago Closed 10 years ago

Incubator qt embedding should switch to QGraphicsWidgets

Categories

(Core Graveyard :: Embedding: APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tatiana, Unassigned)

References

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.8) Gecko/20100723 Ubuntu/10.04 (lucid) Firefox/3.6.8
Build Identifier: 

due to bug #544216 changes we need to switch to QGraphicsWidgets usage

Reproducible: Always
Blocks: 583546
Attachment #465303 - Flags: review?(romaxa)
Attached patch qt test uses QGraphicsWidget (obsolete) — Splinter Review
Attachment #465305 - Flags: review?(romaxa)
Attachment #465303 - Flags: review?(romaxa)
Attachment #465303 - Flags: review?(mark.finkle)
Attachment #465303 - Flags: feedback?(romaxa)
Attachment #465305 - Flags: review?(romaxa)
Attachment #465305 - Flags: review?(mark.finkle)
Attachment #465305 - Flags: feedback?(romaxa)
Comment on attachment 465303 [details] [diff] [review]
QMozView uses QGraphicsWidget

This is looks correct
Attachment #465303 - Flags: feedback?(romaxa) → feedback+
Comment on attachment 465305 [details] [diff] [review]
qt test uses QGraphicsWidget


>+MyQGraphicsView::MyQGraphicsView(const QString &aUri, QGraphicsScene *scene, QWidget *parent)
...
>+    if(argc > 1)
>+        view = new MyQGraphicsView(argv[argc - 1], &scene);
>+    else
>+        view = new MyQGraphicsView("http://mozilla.org", &scene);

Don't really like example of code which is taking URL as argument to QGraphicsView... not good program design style..
Move load Url in separate function, otherwise people will follow to this example and will have problems and confuse about why it was done this way.



The rest looks ok
Attachment #465305 - Flags: feedback?(romaxa) → feedback-
Attached patch qt test patch (obsolete) — Splinter Review
moved load Uri in separate function
Attachment #465305 - Attachment is obsolete: true
Attachment #466421 - Flags: review?(mark.finkle)
Attachment #466421 - Flags: feedback?(romaxa)
Attachment #465305 - Flags: review?(mark.finkle)
Comment on attachment 466421 [details] [diff] [review]
qt test patch

I would like to have have in this file mozilla style:
Instead of 
-    QLabel* status;
+    QGraphicsWidget *mForm;
Should be:
-    QLabel* status;
+    QGraphicsWidget* mForm;

Also here:
+    MyQGraphicsView(QGraphicsScene * scene, QWidget *parent = 0);
should be
+    MyQGraphicsView(QGraphicsScene* scene, QWidget* parent = 0);


And other places:
-+ void consoleMessage(const QString &message);
++ void consoleMessage(const QString& message);



But that is style things... functionality seems to be ok, now
Attachment #466421 - Flags: feedback?(romaxa) → feedback+
Attached patch qt test patch (obsolete) — Splinter Review
ok, using mozilla style now, I wasn't sure about mozilla vs. qt style from original code
Attachment #466421 - Attachment is obsolete: true
Attachment #466448 - Flags: review?(mark.finkle)
Attachment #466421 - Flags: review?(mark.finkle)
Attached patch qt test patchSplinter Review
Attachment #466448 - Attachment is obsolete: true
Attachment #466451 - Flags: review?(mark.finkle)
Attachment #466448 - Flags: review?(mark.finkle)
Comment on attachment 466451 [details] [diff] [review]
qt test patch

Switching review to Oleg
Attachment #466451 - Flags: review?(mark.finkle) → review?(romaxa)
Attachment #465303 - Flags: review?(mark.finkle) → review?(romaxa)
Attachment #465303 - Flags: review?(romaxa) → review+
Comment on attachment 466451 [details] [diff] [review]
qt test patch

ok, this looks lot better
Attachment #466451 - Flags: review?(romaxa) → review+
http://hg.mozilla.org/incubator/embedding/rev/13ee3bfc6a10
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.