Rob Oakes wrote:
Dear LyX Developers,
Attached you will find a patch that adds a basic, Scrivener
(http://www.literatureandlatte.com/scrivener.html) like corkboard to LyX.
If interested more information about what specific features I would like to
implement can be found at my website
(http://www.oak-tree.us/blog/index.php/2009/03/04/perfect-tool). You can
also find a (somewhat) working prototype that I started as a playground in
PyQt (http://www.oak-tree.us/blog/index.php/science-and-technology/lyx-outline).
The general idea looks really good to me. I haven't used Scrivener, as
it's not available for Linux, but it looks quite useful. I'm forever
using note insets for this kind of purpose, and they are not ideal. I
used to use basket (a note-taking app), but I'd rather have the notes
with the document. So something like this would be very useful for me
and, I'm sure, for others.
As for the patch, I haven't had a chance to look at this in any detail,
but I do have a few suggestions to make. One is that "&", which you use
occasionally in if-clauses, is bitwise and, not logical and. You want
"&&". Same for "|" vs "||".
There are a lot of variables in here that could be and so should be
const. (I had to learn this myself when coming to C++.) A const
declaration signals that you do not intend to change the value, and so
the compiler will let you know if you do try to change it. Thus, in here:
+QRect Corkboard::itemRect(const QModelIndex& modelIndex) const {
+ QRect viewCardRect = cardRect(modelIndex);
+ QString text = model()->data(modelIndex).toString();
+ QSize size = textSize(text, viewOptions(), viewCardRect.width(),
viewCardRect.height());
+
+ int col = modelIndex.column();
+
+ if (col == 0) {
+ QPoint titleLeft(viewCardRect.x()+10, viewCardRect.y()+10);
+ return QRect(titleLeft,size);
+ }
+
+ else if (col == 1) {
+ int fontHeight = QFontMetrics(viewOptions().font).height();
+ QPoint synopsisLeft(viewCardRect.x()+10,
viewCardRect.y()+fontHeight+20);
+ return QRect(synopsisLeft,size);
+ }
+
+ else
+ return QRect();
+ }
it looks to me as if almost every variable you declare could be const.
Watch out too for places you can use const refs rather than straight
variables. The utility of const refs here is that (like pointers) they
do not need a copy to be made: You're just referencing an already
existing memory location.
The more important suggestion is that you should read the coding
guidelines in development/coding/ and try to follow them. These make the
code consistent and easier to read.
For example, lines should be no longer than 80 characters, which means
that long comments should look like this:
// Edits an item corresponding to the given index (if it can be edited).
// edit() does not change the current index (since the current index
defines
// ...
// Otherwise, it returns false.
(You can also use /* */, but most of the code is as shown.) Long lines like:
if ((wx > synopsisStart.x()) & (wx < synopsisEnd.x()) & (wy >
synopsisStart.y()) & (wy < synopsisEnd.y()))
will need to be broken up, too, perhaps thus:
if (wx > synopsisStart.x() && wx < synopsisEnd.x()
&& wy > synopsisStart.y() && wy < synopsisEnd.y())
Note that the operator goes on the new line. You don't need the extra
parens there, either.
The brace convention is thus:
void Class::function(type var)
{
...code...
}
For if...else, I'm not sure we're totally consistent, but it's usually:
if (...) {
....
} else {
....
}
and similarly for else ifs.
We do "QPoint const &" rather than "const QPoint&", and we try to be
consistent about spacing around commas, etc.
You'll pick this all up.
Just as a matter of style, I'd replace this:
QPoint px2(p2.x(),
p2.y()+(viewRow*(cardHeight+10))-verticalScrollBar()->value());
with something like:
int const px2yval = p2.y() + (viewRow * (cardHeight + 10)) -
verticalScrollBar()->value();
QPoint px2(p2.x(), px2yval);
(Note the spacing.) This likely costs nothing, as the compiler will
surely optimize away the int. But it's a lot clearer to read.
So I'd suggest you clean up the patch along the lines suggested and
repost. Then it'll be easier for me and others to read it and give
substantive comments. And, of course, to try it out.
Richard
