Summary: | [EFL] Supports to execute "redo" command. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jaehun Lim <ljaehun.lim> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | eric, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, rniwa, sangseok.lim, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | 62726 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Jaehun Lim
2011-06-13 03:00:21 PDT
Created attachment 96942 [details]
Proposed patch
The code looks like the one in the GTK+ port but without some additional checks (for m_isInRedo, for example). I suggest porting those checks as well. > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:150 > + redoStack.clear(); Perhaps it might be good to check if you're not inside a redo operation? > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194 > + RefPtr<WebCore::EditCommand> command(*(--redoStack.end())); What if the stack is empty? You should probably enclose this block of code inside an if (canRedo()) Comment on attachment 96942 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=96942&action=review > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194 > + RefPtr<WebCore::EditCommand> command(*(--redoStack.end())); So this will never be called when canRedo is false I assume? Comment on attachment 96942 [details] Proposed patch Clearing flags on attachment: 96942 Committed r88723: <http://trac.webkit.org/changeset/88723> All reviewed patches have been landed. Closing bug. (In reply to comment #2) > The code looks like the one in the GTK+ port but without some additional checks (for m_isInRedo, for example). I suggest porting those checks as well. > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:150 > > + redoStack.clear(); > > Perhaps it might be good to check if you're not inside a redo operation? > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194 > > + RefPtr<WebCore::EditCommand> command(*(--redoStack.end())); > > What if the stack is empty? You should probably enclose this block of code inside an if (canRedo()) I missed some checking codes as you said. I'll make another patch soon. Thanks. Re-opening this bug. Please address the comments #2 and #3. Created attachment 97375 [details]
Modified patch
(In reply to comment #2) > The code looks like the one in the GTK+ port but without some additional checks (for m_isInRedo, for example). I suggest porting those checks as well. > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:150 > > + redoStack.clear(); > > Perhaps it might be good to check if you're not inside a redo operation? > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:194 > > + RefPtr<WebCore::EditCommand> command(*(--redoStack.end())); > > What if the stack is empty? You should probably enclose this block of code inside an if (canRedo()) I added some codes for status checking using m_isInRedo or canRedo() as you said. It looks OK to me now. Informal r+. Please mark the previous patch as obsolete. (In reply to comment #10) > It looks OK to me now. Informal r+. Please mark the previous patch as obsolete. Sorry, I missed it. Informal review r+ on my side. Comment on attachment 97375 [details] Modified patch View in context: https://bugs.webkit.org/attachment.cgi?id=97375&action=review > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:157 > - notImplemented(); > + redoStack.append(command); Why don't you prepend here instead? > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:190 > + RefPtr<WebCore::EditCommand> command(*(--undoStack.end())); > + undoStack.remove(--undoStack.end()); so that you can use undoStack.first() and undoStack.removeFirst() here instead. Created attachment 100248 [details]
Modified patch
Use prepend() instead of append().
(In reply to comment #13) > (From update of attachment 97375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97375&action=review > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:157 > > - notImplemented(); > > + redoStack.append(command); > > Why don't you prepend here instead? > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:190 > > + RefPtr<WebCore::EditCommand> command(*(--undoStack.end())); > > + undoStack.remove(--undoStack.end()); > > so that you can use undoStack.first() and undoStack.removeFirst() here instead. I applied your comments. Thanks. Created attachment 100249 [details]
Fix a trivial mistake.
Comment on attachment 100249 [details] Fix a trivial mistake. View in context: https://bugs.webkit.org/attachment.cgi?id=100249&action=review > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:203 > + RefPtr<WebCore::EditCommand> command(undoStack.takeFirst()); Nit: I think we normally use assignment operator here. > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:211 > + RefPtr<WebCore::EditCommand> command(redoStack.takeFirst()); Ditto. > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214 > + ASSERT(!m_isInRedo); > + m_isInRedo = true; Do we really need to do this? We don't do this for undo, right? (In reply to comment #17) > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214 > > + ASSERT(!m_isInRedo); > > + m_isInRedo = true; > > Do we really need to do this? We don't do this for undo, right? This one was probably my suggestion, as this is exactly what the gtk port does. It shouldn't hurt IMO. Created attachment 100375 [details]
Patch
Comment on attachment 100375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100375&action=review > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214 > + ASSERT(!m_isInRedo); > + m_isInRedo = true; I'm still not happy about the fact there's asymmetry between undo and redo. (In reply to comment #20) > (From update of attachment 100375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100375&action=review > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214 > > + ASSERT(!m_isInRedo); > > + m_isInRedo = true; > > I'm still not happy about the fact there's asymmetry between undo and redo. It looks the m_isInRedo flag is used by registerCommandForUndo() in GTK port. http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp#L354 So, if we don't know why redoStack is cleared when m_isInRedo is false, we should not add the m_isInRedo to redo(). (In reply to comment #21) > (In reply to comment #20) > > (From update of attachment 100375 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=100375&action=review > > > > > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:214 > > > + ASSERT(!m_isInRedo); > > > + m_isInRedo = true; > > > > I'm still not happy about the fact there's asymmetry between undo and redo. > > It looks the m_isInRedo flag is used by registerCommandForUndo() in GTK port. > > http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp#L354 > > So, if we don't know why redoStack is cleared when m_isInRedo is false, we should not add the m_isInRedo to redo(). I think we should check redo status when a command is added into undoStack. In general, when a new command is added into undoStack, commands in redoStack are useless, So redoStack should be cleared. But when a command is moved from redoStack to undoStack by redo function, we should not clear the redoStack, because user could redo next commands in redoStack continuously. But current registerCommandForUndo() has no redostack clearing codes. I'll make a new patch. Created attachment 100436 [details]
New patch
Comment on attachment 100436 [details]
New patch
LGTM. Ryuske, could you review this patch again ?
Comment on attachment 100436 [details] New patch View in context: https://bugs.webkit.org/attachment.cgi?id=100436&action=review > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:167 > + if (!m_isInRedo) > + redoStack.clear(); Now, this makes a lot of sense! > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:218 > + m_isInRedo = true; > + command->reapply(); > + m_isInRedo = false; I must say that this is likely broken if scripts run execCommand in mutation events but I suppose all other ports have the same issue ? :( Comment on attachment 100436 [details] New patch Clearing flags on attachment: 100436 Committed r90808: <http://trac.webkit.org/changeset/90808> All reviewed patches have been landed. Closing bug. |