GNOME Bugzilla – Bug 644161
EphyHome actions cleanup
Last modified: 2012-01-18 12:40:27 UTC
Current EphyHomeAction defines a lot of codepaths becausee it is used for three different actions : Home, NewTab, NewWindow. This make the code hard to read and unefficient. The set of patchs attached try to improve the situation by separating those three aspects: - Pass the defaults flags (new tab, window at construction time - Make the Home action a "pure" EphyLinkAction pointing to the home url. - Make NewTab and NewWindow instances of a new EphyLinkAction subclass called EphyDropTargetLinkAction -remove EphyHomeAction
Created attachment 182785 [details] [review] EphyHomeAction cleanup Make the Home action a pure LinkAction pointing to the home url.
Created attachment 182786 [details] [review] EphyHome actions cleanup Make NewTab and NewWindow instances of a new EphyLinkAction subclass called EphyDropTargetLinkAction. This class support links dropped on them.
Created attachment 182787 [details] [review] EphyHome actions cleanup Remove unused EphyHomeAction
Review of attachment 182786 [details] [review]: I'm not 100% sure what this change gets us to be honest - would be good to have more description of the why in the commit message as opposed to the what, but the code looks good to me otherwise. I would totally not have "Remove unused EphyHomeAction" as a separate patch. It belongs in this patch IMO, since what's effectively going on is essentially a rename of EphyHomeAction to EphyDropppableLinkAction. ::: src/ephy-droppable-link-action.h @@ +41,3 @@ +struct _EphyDroppableLinkAction +{ + EphyLinkAction parent_instance; The .c file has 2-space indentation, should keep the .h file consistent =)
Hi Kov, Thanks for you review (In reply to comment #4) > Review of attachment 182786 [details] [review]: > > I'm not 100% sure what this change gets us to be honest - would be good to have > more description of the why in the commit message as opposed to the what Current EphyHome action is ifed everywhere as it must handle the Home, NewTab and NewWindow actions. The Home action can (and should IMHO) be handle by a pure EphyLinkAction set with the right URL. EphyLinkAction became cleverer with the patchs attached to bug 628364. The two others share a LOT but there default opening flags, here again, the new EphyLinkAction takes care of that. The common supplementary behaviour compared to EphyLinkAction is their ability to act as drop targets. Thus the name, making the intent clearer. > , but > the code looks good to me otherwise. I would totally not have "Remove unused > EphyHomeAction" as a separate patch. It belongs in this patch IMO, since what's > effectively going on is essentially a rename of EphyHomeAction to > EphyDropppableLinkAction. "essentially" yep. But only remains here the drop-target behaviour. > > ::: src/ephy-droppable-link-action.h > @@ +41,3 @@ > +struct _EphyDroppableLinkAction > +{ > + EphyLinkAction parent_instance; > > The .c file has 2-space indentation, should keep the .h file consistent =) Yup ! Thanks !
EphyHomeAction is really tiny now, since it only handles the FileNewTab action from the super menu. I think we can close this, thanks for the patches!