Closed Bug 586722 Opened 14 years ago Closed 14 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+
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: