Summary: | [Qt] QWebView, QGraphicsWebView and QWebFrame setHtml() should be better documented | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jędrzej Nowacki <jedrzej.nowacki> | ||||||||
Component: | WebKit Qt | Assignee: | Alexis Menard (darktears) <menard> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hausmann, tonikitoo, vestbo | ||||||||
Priority: | P3 | Keywords: | Qt | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 29457, 29461, 29876, 31552 | ||||||||||
Attachments: |
|
Description
Jędrzej Nowacki
2009-11-04 06:20:39 PST
I agree that merging setHtml and setContent might be a good thing to be done at some point. While I agree that setHtml() may not be a very "pure" function to have (give the XHTML relation you mentioned for example), I still think it is a good thing to have. * The function is consistent with the rest of Qt (QTextEdit::setHtml, QTextDocument::setHtml, QGraphicsTextItem::setHtml). Qt developers love the fact that our API is consistent. The unspoken guideline is that an experienced Qt developer shouldn't have to read the documentation of a class in order to use it. That is why completely different classes like QTabBar, QComboBox or QListWidget have a count() function. They're not called numTabs(), numItems() or numListItems(), which would be a slightly tighter name. Instead they share the same name so that the API remains concise. * setHtml() is also very convenient. We say that with the Qt API common things should be easy and special tasks should be possible. Populating a widget to render web content with a given string of HTML falls clearly into the former category. It is less error prone than calling setContent(htmlString, "text/htlm"). Oh, oops, that should've been "text/html", but I only found out at run-time because my text didn't show :) * To satisfy the use-case of a more advanced task we added setContent(), which is harder to use but also more flexible. Created attachment 43851 [details] documentation change v1 (In reply to comment #2) > * The function is consistent with the rest of Qt (QTextEdit::setHtml, > QTextDocument::setHtml, QGraphicsTextItem::setHtml). Qt developers love the > fact that our API is consistent. This is nice point :-), but on the other hand classes mentioned by you are much simpler (implementation & functionality). I think it's clear that you shouldn't put SVG into the QTextEdit, but for the QWebPage it is not so obvious. Moreover Qt had to be not only simple, but should "force" right application design. As setHtml is a nice shortcut, I don't think that it must be used on the first place. Developers need to know what actually they want to process (SVG, HTML, XHTML...). > * setHtml() is also very convenient. We say that with the Qt API common things > should be easy and special tasks should be possible. Populating a widget to > render web content with a given string of HTML falls clearly into the former > category. Yes it should be, but as we don't provide unified access for XHTML and HTML it is a source of problems. > * To satisfy the use-case of a more advanced task we added setContent(), which > is harder to use but also more flexible. So I prepared a little patch for it. I don't think, we should close the discussion. jedrzej, i like the docs and think this change is enough to sort the confusion out. I just think you should change QGraphicsWebView's setHtml content docs too :-) in WebKit/qt/Api/qwebview.cpp changing bug subject accordingly to the direction the solution is taking ... Created attachment 62944 [details]
Patch for the doc fix, with QGraphicsWebView as well
Comment on attachment 62944 [details]
Patch for the doc fix, with QGraphicsWebView as well
WebKit/qt/Api/qgraphicswebview.cpp:715
+ setContent() should be used instead
Misses a dot at the end - both places it is used.
WebKit/qt/Api/qgraphicswebview.cpp:714
+ \warning This function works only for HTML, for other mime types (ex. XHTML, SVG)
I would use 'ie.' instead of ex. I believe that is more English.
Created attachment 62946 [details]
Patch for the doc fix, with QGraphicsWebView as well - v2
Comment on attachment 62946 [details]
Patch for the doc fix, with QGraphicsWebView as well - v2
r=me
*** Bug 29461 has been marked as a duplicate of this bug. *** Comment on attachment 62946 [details] Patch for the doc fix, with QGraphicsWebView as well - v2 Clearing flags on attachment: 62946 Committed r64284: <http://trac.webkit.org/changeset/64284> All reviewed patches have been landed. Closing bug. Revision r64284 cherry-picked into qtwebkit-2.1 with commit 20c90a1df97b9ca403a562e19624b2245de4c09e |