Closed
Bug 586722
Opened 14 years ago
Closed 14 years ago
Incubator qt embedding should switch to QGraphicsWidgets
Categories
(Core Graveyard :: Embedding: APIs, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: tatiana, Unassigned)
References
Details
Attachments
(2 files, 3 obsolete files)
2.62 KB,
patch
|
romaxa
:
review+
romaxa
:
feedback+
|
Details | Diff | Splinter Review |
8.73 KB,
patch
|
romaxa
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•14 years ago
|
||
Attachment #465303 -
Flags: review?(romaxa)
Reporter | ||
Comment 2•14 years ago
|
||
Attachment #465305 -
Flags: review?(romaxa)
Reporter | ||
Updated•14 years ago
|
Attachment #465303 -
Flags: review?(romaxa)
Attachment #465303 -
Flags: review?(mark.finkle)
Attachment #465303 -
Flags: feedback?(romaxa)
Reporter | ||
Updated•14 years ago
|
Attachment #465305 -
Flags: review?(romaxa)
Attachment #465305 -
Flags: review?(mark.finkle)
Attachment #465305 -
Flags: feedback?(romaxa)
Comment 3•14 years ago
|
||
Comment on attachment 465303 [details] [diff] [review]
QMozView uses QGraphicsWidget
This is looks correct
Attachment #465303 -
Flags: feedback?(romaxa) → feedback+
Comment 4•14 years ago
|
||
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-
Reporter | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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+
Reporter | ||
Comment 7•14 years ago
|
||
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)
Reporter | ||
Comment 8•14 years ago
|
||
Attachment #466448 -
Attachment is obsolete: true
Attachment #466451 -
Flags: review?(mark.finkle)
Attachment #466448 -
Flags: review?(mark.finkle)
Comment 9•14 years ago
|
||
Comment on attachment 466451 [details] [diff] [review]
qt test patch
Switching review to Oleg
Attachment #466451 -
Flags: review?(mark.finkle) → review?(romaxa)
Updated•14 years ago
|
Attachment #465303 -
Flags: review?(mark.finkle) → review?(romaxa)
Updated•14 years ago
|
Attachment #465303 -
Flags: review?(romaxa) → review+
Comment 10•14 years ago
|
||
Comment on attachment 466451 [details] [diff] [review]
qt test patch
ok, this looks lot better
Attachment #466451 -
Flags: review?(romaxa) → review+
Comment 11•14 years ago
|
||
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•