Closed scunz closed 11 years ago
@antis81, could you please review this and merge it, if it's okay?
This also simplifies conversion of HistoryView
to a ContextView
.
Out of those 3 options, none is anything like appealing to me:
QTableView
approach is hackish. And in fact you can't do much more with it than what is possible by using QTextBrowser
. This would really really be a last resort solution in my book. Actually I'd rather go and spend a whole week fixing the rendering and painting code we had before... But a whole week is out of scope at the current stage. See libHeaven's Issue 11 as a reference what roughly one week fulltime is worth... :-) By the way, there is a reason, why libHeaven even implements its own QStyle derivat - and that reason is not that I read it can be done like that in the QtCreator sources... It's simply that qss is a technique that has more issues than benefits.QGraphicsView
approach... Well, hum, did you already read the libDiffViews issue wrt. the sequential diff view and QGraphicsView
? It's a total mess. I've reread the code about a dozen times now - and I still fail to see any fundamental flaw in those calculations. Yet it overdraws and the font positioning, even for Qt4, deserves the term experimental compromise. The font positioning for Qt5 on Linux (xcb) is absolutely broken. I cant fix it, it's an upstream problem of Qt - and they seem not to provide a fix for Qt5.1, so it will probably be yet another 6 months before this will be usable.QWebView
is the only considerable alternative. When it comes to rendering HTML fragments inside our application, WebKit is the only modern way to go. However, depending on WebKit has one problem: While on Linux, distributions will simply add a flag "needs qwebkit" to the RPM, distribution on Windows and Mac doesn't work like that. In the Qt5 world, QWebKit demands a Qt-base build with ICU. We'd get the additional dependencies to ship: ICU + WebKit + QtScript. Alone the data DLL of icu is 20 MiB(!). Thus, this brings roughly 50 MiB increase to an installer for windows or an App-Bundle. That is what we don't want.The font for the header (commit short message) is a little too big. 1 or 2pt smaller and it would go great.
It is relative to the application wide font, actually. I tried with font-size: large;
but that didn't result in any difference from omitting it. Maybe we should just go with the default size and make that subject line bold.
Minor: just keep the underline for the header and remove any borders.
I don't really understand that. However, I'm not too closely looking at how it will be looking. I'm currently way more into functionality. So as a bottom line, I'd say: check the code out, prettify the template at the bottom of the .cpp file, commit to the PR and merge it. (I'd also be okay with reversed order :-) )
As said already, this is highly cosmetic. The code looks very ok to me. So I simply merge it without any changes. Just to get an impression on how it would look like (bold font and no border):
Thanks, so this is in, I can continue with the ViewContextData
migration for ModHistory.
Btw, I'm preparing 2 new Modules:
Not sure yet, if they will finally be based on development or uncrappify branches.
This change is a total rewrite of the current history view.
Instead of drawing it all ourselves, setup some HTML and show that in a browser. This addresses most of the details related topics in macgitver/MacGitver#3:
Snapshot from a Linux and Qt4 based build: