Dr. Marmorstein,
Thanks for taking the time to look at it. I know it wasn't the cleanest patch,
and the code I threw up on github ( http://github.com/synfin/Basket-Undo）could
have been better.
1.-4. Will do.
5. Understood. That was a "hack", I was trying to get it work and I literally
copied and pasted a few elements. I cannot remember my exact reasoning for not
calling pasteNote directly, there was something it did internally that was
incompatible with what I was trying to do. Or at least, I couldn't figure out
a simple way to call it with heavier rewrites. I'll have to review that.
I hadn't really decided on having "two different views" for undo, that was a
mistake. I would prefer a global undo system, and not enforcing two different
views for undo. The trivial patch I wrote enforces two views, which I
personally dislike.
I'll have to think on that for a bit. I'll move forward with this, trying to
produce a patch that is truly functional and not proof of concept. I expect a
few revisions afterwards and changes to merge with everyone else's submission
of course.
Thanks!
-Syn2Fin
--- On Thu, 3/4/10, Dr. Robert Marmorstein <rob...@narnia.homeunix.com> wrote:
From: Dr. Robert Marmorstein <rob...@narnia.homeunix.com>
Subject: Re: [Basket-devel] Undo / Redo Patch
To: basket-devel@lists.sourceforge.net
Date: Thursday, March 4, 2010, 3:48 AM
Hi Syn,
Okay. Finally had a moment to breathe and looked over your patch. For the
most part it looks good, but I have a couple of comments and a few questions:
1. When you submit this for real, please remove all the printf calls. If you
want to do real debugging, you can use either KDebug (recommended) or QDebug
calls instead.
2. Similarly, please take out all of the commented out code. These make it
much harder to see what's really changed by cluttering up the diff.
3. Another thing to avoid is introducing "whitespace" changes, like the ones
to basketview.h.
4. I don't think you need to add the "setupUndo" function. Ignoring all of
the commented out stuff, there are only five or six lines of code there and
they
all fit logically into the "setupActions" function that already exists.
5. Do you really need to close the editor and unselect everything in order to
paste the deleted note back in? My guess is that you're copying code from the
"pasteNote" function in BasketView. Is there any reason not to simply call
this function directly? (I haven't thought this through all the way, but
hopefully you can help me see the reasoning).
I think, as a user, I would be surprised if I hit "Undo" and suddenly my
editor closed . . . especially if I was in the middle of editing something.
Playing around with other applications that undo (kspread for instance), it
seems like the convention is to have two different levels of Undo. When you
are in the editor, you can only undo actions you've taken in the editor. Then
when you leave the editor, you enter a mode in which a singe "undo" reverses
everything you just did in the editor and you can undo deletes, adds, etc. Is
that what this code enforces?
I think that's pretty much it. This is a fairly straightforward patch. I
think you've done some EXCELLENT work here. It will be nice to see this
carried forward with "add", "change font", "edit", "tag", and "untag" items
(and so forth).
Thanks for your hard work on this!
Robert
On Monday 22 February 2010 12:03:51 am Syn Fin wrote:
> Greetings all,
>
> I sat down this weekend and wrote a proof of concept patch for including
> undo/redo functionality. It's ONLY purpose is to show the rough idea of
> what needs to be done.
>
> The only feature that works is delete, you can undo a delete and redo a
> delete. It is a hack for the moment and doesn't always put things back
> where they should be (there are good reasons too, please read on). I did
> not do more yet, as it will require a sizeable refactoring of the
> bnpview.{h|cpp} and basketview.{h|cpp} to make it work with the Qt undo
> framework.
>
> Using the Qt framework, all actions that we want to be put on the undo
> stack must be abstracted by an object (DeleteItem, AddItem, MoveItem,
> GroupItems, UngroupItems, etc.) that inherits from QUndoCommand. Each of
> these objects will have a redo() and undo() method with sufficient
> knowledge to roll forward and backwards through the system state.
> However, the code as it is written now, doesn't support this. It will
> probably also require a few extra methods in the Note class.
>
> I have no problem implementing this. But before I spend a lot of time on
> this, please glance at the patch for a brief taste of what I'm proposing
> (the patch is a bit messy, sorry guys, I'm new to this). If people are
> not adamantly opposed, I can write up a fairly short document describing
> what needs to be changed, what new objects need to be added, as well as a
> description of how the redo/undo functionality should work. From there
> comments can be generated and hopefully we'll come to an agreement and
> I'll build from that.
>
> -Syn2Fin
>
> P.S.: I am very sorry for my long hiatus. School + work (and the
> occasional female...) sucked away my time. I have since graduated, and
> time is becoming more readily available.
>
> =====PATCH=====
> diff --git a/src/basket_part.rc b/src/basket_part.rc
> index 4537f1a..a4ac7a7 100644
> --- a/src/basket_part.rc
> +++ b/src/basket_part.rc
> @@ -35,6 +35,8 @@
> </Menu>
> <Menu name="edit" >
> <text>&Edit</text>
> + <Action name="edit_undo" />
> + <Action name="edit_redo" />
> <Action name="edit_cut" />
> <Action name="edit_copy" />
> <Action name="edit_paste" />
> @@ -118,6 +120,8 @@
> <text>Main Toolbar</text>
> <Action name="basket_properties" />
> <Separator lineSeparator="true" name="separator_0" />
> + <Action name="edit_undo" />
> + <Action name="edit_redo" />
> <Action name="edit_cut" />
> <Action name="edit_copy" />
> <Action name="edit_paste" />
> @@ -207,6 +211,8 @@
> <Action name="note_group" />
> <Action name="note_ungroup" />
> <Separator/>
> + <Action name="edit_undo" />
> + <Action name="edit_redo" />
> <Action name="edit_cut" />
> <Action name="edit_copy" />
> <Action name="edit_delete" />
> diff --git a/src/basketui.rc b/src/basketui.rc
> index bdcf7a5..a581ed4 100644
> --- a/src/basketui.rc
> +++ b/src/basketui.rc
> @@ -38,6 +38,8 @@
> </Menu>
> <Menu name="edit" >
> <text>&Edit</text>
> + <Action name="edit_undo" />
> + <Action name="edit_redo" />
> <Action name="edit_cut" />
> <Action name="edit_copy" />
> <Action name="edit_paste" />
> @@ -122,6 +124,8 @@
> <Action name="basket_new_menu" />
> <Action name="basket_properties" />
> <Separator lineSeparator="true" name="separator_0" />
> + <Action name="edit_undo" />
> + <Action name="edit_redo" />
> <Action name="edit_cut" />
> <Action name="edit_copy" />
> <Action name="edit_paste" />
> @@ -214,6 +218,8 @@
> <Action name="note_group" />
> <Action name="note_ungroup" />
> <Separator/>
> + <Action name="edit_undo" />
> + <Action name="edit_redo" />
> <Action name="edit_cut" />
> <Action name="edit_copy" />
> <Action name="edit_delete" />
> diff --git a/src/basketview.h b/src/basketview.h
> index 61d2a7b..8949716 100644
> --- a/src/basketview.h
> +++ b/src/basketview.h
> @@ -416,13 +416,13 @@ public:
> NoteSelection* selectedNotes();
>
> /// BLANK SPACES DRAWING:
> -private:
> + private:
> QList<QRect> m_blankAreas;
> void recomputeBlankRects();
> QWidget *m_cornerWidget;
> -
> +
> /// COMMUNICATION WITH ITS CONTAINER:
> -signals:
> + signals:
> void postMessage(const QString &message); /// << Post a temporar
> message in the statusBar. void setStatusBarText(const QString &message);
> /// << Set the permanent statusBar text or reset it if message isEmpty().
> void resetStatusBarText(); /// << Equivalent to
> setStatusBarText(""). diff --git a/src/bnpview.cpp b/src/bnpview.cpp
> index 5fbb3f0..bdbc84e 100644
> --- a/src/bnpview.cpp
> +++ b/src/bnpview.cpp
> @@ -88,7 +88,7 @@
> #include "notefactory.h"
> #include "notecontent.h"
> #include "unistd.h" // usleep
> -
> +#include "noteselection.h"
>
> #include "bnpviewadaptor.h"
> /** class BNPView: */
> @@ -813,7 +813,8 @@ void BNPView::setupActions()
> #endif
>
> /** Edit :
> ****************************************************************/ -
> + /* synfin edit to include undo/redo functionality */
> + setupUndo(a,ac);
> //m_actUndo = KStandardAction::undo( this,
> SLOT(undo()), actionCollection() );
> //m_actUndo->setEnabled(false); // Not yet implemented !
> //m_actRedo = KStandardAction::redo( this,
> SLOT(redo()), actionCollection() ); @@ -886,6 +887,36 @@
> void BNPView::setupActions()
> a->setText(i18n("&Welcome Baskets"));
> }
>
> +/**
> + * syn2...@yahoo.com
> + * The actual code in bnpview is convoluted with items scattered
> everywhere + * I have given up figuring out where all the connect and
> QAction statements + * are located, and this method handles everything
> related to undo/redo + *
> + * Part of this is also my relative newness to the Basket code and QT
> Framework + * itself. This will be merged with the rest of the code after
> acceptance and + * my own comfort level with Basket's source increases.
> + */
> +void BNPView::setupUndo(KAction *a, KActionCollection *ac) {
> + m_undostack = new QUndoStack();
> + //a = ac->addAction("undo", this, SLOT(undo()));
> + m_actUndo=ac->addAction(KStandardAction::Undo,this,SLOT(undo()));
> + //a->setText(i18n("&undo"));
> + //a->setShortcut(0);
> + //m_actUndo=a;
> + m_actRedo=ac->addAction(KStandardAction::Redo,this,SLOT(redo()));
> + //a = ac->addAction("redo", this, SLOT(redo()));
> + //a->setText(i18n("&redo"));
> + //a->setShortcut(0);
> + //m_actRedo=a;
> +
> + // Connect appropriate items
> +
> connect(m_undostack,SIGNAL(canUndoChanged(bool)),m_actUndo,SLOT(setEnabled
> (bool))); +
> connect(m_undostack,SIGNAL(canRedoChanged(bool)),m_actRedo,SLOT(setEnabled
> (bool))); +
> connect(m_actUndo,SIGNAL(triggered()),m_undostack,SLOT(undo())); +
> connect(m_actRedo,SIGNAL(triggered()),m_undostack,SLOT(redo())); +}
> +
> BasketListViewItem* BNPView::topLevelItem(int i)
> {
> return (BasketListViewItem *)m_tree->topLevelItem(i);
> @@ -1592,7 +1623,10 @@ void BNPView::copyNote()
> }
> void BNPView::delNote()
> {
> - currentBasket()->noteDelete();
> + //currentBasket()->noteDelete();
> + printf("You called BNPView::delNote()\n");
> + QUndoCommand * delItem = new DeleteItem(currentBasket());
> + m_undostack->push(delItem);
> }
> void BNPView::openNote()
> {
> @@ -2042,11 +2076,13 @@ void BNPView::setFiltering(bool filtering)
> void BNPView::undo()
> {
> // TODO
> + printf("In BNPView::undo()\n");
> }
>
> void BNPView::redo()
> {
> // TODO
> + printf("In BNPView::redo()\n");
> }
>
> void BNPView::pasteToBasket(int /*index*/, QClipboard::Mode /*mode*/)
> @@ -2811,3 +2847,34 @@ void BNPView::disconnectTagsMenuDelayed()
> disconnect(m_lastOpenedTagsMenu, SIGNAL(aboutToHide()),
> currentBasket(), SLOT(unlockHovering())); disconnect(m_lastOpenedTagsMenu,
> SIGNAL(aboutToHide()), currentBasket(), SLOT(disableNextClick())); }
> +
> +DeleteItem::DeleteItem(BasketView *currentBasket, QUndoCommand *parent) :
> QUndoCommand(parent) { + this->currentBasket=currentBasket;
> + NoteSelection *selection = currentBasket->selectedNotes();
> + if (selection->firstStacked()) {
> + d = NoteDrag::dragObject(selection, true, /*source=*/0);
> + mimeData = d->mimeData();
> + currentBasket->noteDelete();
> + }
> +}
> +void DeleteItem::undo() {
> + printf("in DeleteItem::undo()");
> + //The trick is to "paste" the note where it used to be.
> + currentBasket->closeEditor();
> + currentBasket->unselectAll();
> + Note *note = NoteFactory::dropNote(mimeData,currentBasket);
> + if (note) {currentBasket->insertCreatedNote(note);}
> + else {printf("Note was null\n");}
> +}
> +void DeleteItem::redo() {
> + currentBasket->noteDelete();
> + printf("in DeleteItem::redo()");
> +}
> +//AddItem::AddItem() {
> +//}
> +void AddItem::undo() {
> + printf("in AddItem::undo()");
> +}
> +void AddItem::redo() {
> + printf("in AddItem::redo()");
> +}
> diff --git a/src/bnpview.h b/src/bnpview.h
> index ecd6057..140eed4 100644
> --- a/src/bnpview.h
> +++ b/src/bnpview.h
> @@ -32,6 +32,8 @@
> #include <QHideEvent>
> #include <QEvent>
> #include <QShowEvent>
> +#include <QUndoStack>
> +#include <QUndoCommand>
>
> #include "global.h"
> #include "basket_export.h"
> @@ -59,6 +61,8 @@ class BasketStatusBar;
> class Tag;
> class State;
> class Note;
> +class DeleteItem;
> +class AddItem;
>
> class BASKET_EXPORT BNPView : public QSplitter
> {
> @@ -332,6 +336,7 @@ private slots:
> void slotContextMenu(const QPoint &pos);
> void slotShowProperties(QTreeWidgetItem *item);
> void initialize();
> + void setupUndo(KAction *a,KActionCollection *ac);
>
> signals:
> void basketNumberChanged(int number);
> @@ -345,6 +350,7 @@ protected:
>
> private:
> BasketTreeListView *m_tree;
> + QUndoStack *m_undostack;
> QStackedWidget *m_stack;
> bool m_loading;
> bool m_newBasketPopup;
> @@ -364,4 +370,20 @@ private:
> QTimer *m_hideTimer;
> };
>
> +class DeleteItem : public QUndoCommand {
> + public:
> + DeleteItem(BasketView * currentBasket, QUndoCommand *parent = 0);
> + void undo();
> + void redo();
> + private:
> + BasketView *currentBasket;
> + QDrag *d;
> + QMimeData * mimeData;
> +};
> +class AddItem : public QUndoCommand {
> + public:
> + void undo();
> + void redo();
> +};
> +
> #endif // BNPVIEW_H
-----Inline Attachment Follows-----
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
-----Inline Attachment Follows-----
_______________________________________________
Basket-devel mailing list
Basket-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/basket-devel
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Basket-devel mailing list
Basket-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/basket-devel
