On Sat, Oct 19, 2002 at 11:37:32PM +0200, Lars Gullik Bjønnes wrote:
> | + // bool succes = // usused
> |
> | YUCK !
>
> The misspelling you mean?
> (I think this code is being worked on, so I did not really want to
> change anything... but it gave compiler warnings and I just cannot
> stand that...)
Then change the code to this :
> | // FIXME: do we need to use return value from classApply() here
> | // ??
> | classApply();
which indicates that perhaps we need to consider return value, and is
future proof. Imagine somebody new coming across "// boo succes = //
usused" - it means NOTHING to them.
This is a personal peeve of mine - mainly because I have had this exact
same problem with tons of core lyx code :(
> | -void QCommandBuffer::complete_selected(const QString & str)
> | +void QCommandBuffer::complete_selected(QString const & str)
> |
> | You've tested this part is safe ?
>
> how to test?
type "ma" in minibuffer, press right arrow, select an entry.
> | + void complete_selected(QString const & str);
>
> This must be safe... if this isn't safe I am going to shoot someone...
I think it would be safe.
> | If you're going to get rid of the debugging comments while it's still
> | being worked on just remove them :)
>
> qWarning("%d", s) does not compile cleanly... s is QString...
> (so how can it give valid debug info?)
then please remove it, don't comment it. We are using CVS, remember.
> | - QLyXKeySym const & o = static_cast<QLyXKeySym const &>(k);
> | - // ignore text_ !
> | - return o.key_ == key_;
> | + return static_cast<QLyXKeySym const &>(k1).key()
> | + == static_cast<QLyXKeySym const &>(k2).key();
> | +
> |
> | Please don't remove this comment - it is unusual behaviour.
>
> Then perhaps the comment should spell it out a bit better.
Probably.
// note we ignore text_ here (non-strict ==), because
// text_ is not filled out by keymap initialisation
> | - // Yes, it is this ugly. Two reasons - root items must have a QListView
>parent,
> | - // rather than QListViewItem; and the TOC can move in and out an arbitrary
>number
> | - // of levels
> | + // Yes, it is this ugly. Two reasons - root items must have
> | + // a QListView parent,
> | + // rather than QListViewItem; and the TOC can move in and
> | + // out an arbitrary number of levels
> |
> | Why did you fubar the formatting ?
>
> Some stupid ass used +80 char lines...
Yes I am stupid ass. But where is emacs format-paragraph ?? Doesn't it
work :)
// Yes, it is this ugly. Two reasons - root items must have a
// QListView parent, rather than QListViewItem; and the TOC can
// move in and out an arbitrary number of levels
That is the result of "gqap" in vim
> | + // FIX! (Lgb)
> | +
>
> the key translating? it should use a map or something so that binary
> search could be used.... now it is tens and tens of comparisons for
> the entries close to the bottom.
OK, sure. This stuff is still under heavy tweaking, I haven't cleaned
up yet.
> | + // bitBlt(dest, dest_x, dest_y, source, src_x, src_y, src_w, src_h)
> | + bitBlt(p, 0, dest_y, p, 0, src_y, src_w, src_h);
>
> do not add comments at end of line... but since someone wanted to know
> how to call bitBlt I thought I should not remove that info completely.
This looks confusingly like you might swap the code around ... but no
big deal
> And I had a lot of fun... but the Qt code was in a lot better shape
> than I would have thought.... good work.
There's still a lot more cleanup I think we can do ;)
regards
john
--
"It's a cardboard universe ... and if you lean too hard against it, you fall
through."
- Philip K. Dick
