Code review comment for lp:~hid-iwata/qbzr/qshelve

Revision history for this message
Alexander Belchenko (bialix) wrote :

I really want to see it merged very soon. I have some feedback regarding UI. Some background: one of the good features of QBzr is consistency between different q-windows. That's not ideal though, but we should try to improve this consistency. That means we should follow create major control elements in similar way in the different windows.

1) So, let's look at you toolbar: http://dl.dropbox.com/u/16802579/shelve.png

It's slightly different from qdiff toolbar in the way it displays options (Encoding and Complete). Your toolbar put both options directly on toolbar, while qdiff puts them into View Options submenu: http://bazaarvcs.files.wordpress.com/2011/04/qdiff-options.png

I'd like you put your options into similar submenu as well, or even better reuse that menu if possible (hiding unneeded options), so we can extend options in one place.

Your icons for that options are very nice, so we can use them in qdiff too.

2) I suggest using more descriptive names for button Next and Previous, as Next hunk / Previous Hunk, or Next Change / Previous Change. That's because when Find panel is open user will see 2 pairs of next/previous and that's confusing: http://dl.dropbox.com/u/16802579/shelve-find.png

3) You have added only one new command: qshelve. I think we need qunshelve as well to match bzr CLI. That should be similar to http://dl.dropbox.com/u/16802579/shelvelist.png but I think we don't really need `qshelve --list` then. Or you can retain `qshelve --list` but makes it alias to qunshelve window.

« Back to merge proposal