----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104882/#review13578 -----------------------------------------------------------
Ok I've added a lot of comments, but I also have trouble understanding the "spaghetti" of signals, bools and names in the StyleManager. It must be possible to do this in a more elegant way. plugins/textshape/dialogs/CharacterGeneral.cpp <http://git.reviewboard.kde.org/r/104882/#comment10756> this line should be removed completely plugins/textshape/dialogs/CharacterGeneral.cpp <http://git.reviewboard.kde.org/r/104882/#comment10755> you should probably disconnect the new connection you have made. In any case this lins should be removed completely plugins/textshape/dialogs/ParagraphGeneral.cpp <http://git.reviewboard.kde.org/r/104882/#comment10757> this line should be removed plugins/textshape/dialogs/StyleManager.cpp <http://git.reviewboard.kde.org/r/104882/#comment10758> why? plugins/textshape/dialogs/StyleManager.cpp <http://git.reviewboard.kde.org/r/104882/#comment10759> why? plugins/textshape/dialogs/StyleManager.cpp <http://git.reviewboard.kde.org/r/104882/#comment10760> please remove space plugins/textshape/dialogs/StyleManager.cpp <http://git.reviewboard.kde.org/r/104882/#comment10761> please write: "There already exists another style with this name. Please choose another name" plugins/textshape/dialogs/StyleManager.cpp <http://git.reviewboard.kde.org/r/104882/#comment10762> please write: "There already exists another style with this name. Please choose another name" plugins/textshape/dialogs/StyleManager.cpp <http://git.reviewboard.kde.org/r/104882/#comment10763> please write: "There already exists another style with this name. Please choose another name" plugins/textshape/dialogs/StyleManager.cpp <http://git.reviewboard.kde.org/r/104882/#comment10764> please no blank line here plugins/textshape/dialogs/StyleManagerDialog.cpp <http://git.reviewboard.kde.org/r/104882/#comment10765> hmm, while I agree we should warn, the user trying to leave unapplied changes it should be a choise dialog. And it shouldn't be just changing the name but any change. plugins/textshape/dialogs/StyleManagerDialog.cpp <http://git.reviewboard.kde.org/r/104882/#comment10766> Same thing as in reject. In fact i'm not even sure it's needed to handle the close event. Im pretty sure it automatically calls reject() plugins/textshape/dialogs/StylesModel.h <http://git.reviewboard.kde.org/r/104882/#comment10767> Please add apidox for this new method. - C. Boemann On May 7, 2012, 10:21 p.m., mojtaba shahi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104882/ > ----------------------------------------------------------- > > (Updated May 7, 2012, 10:21 p.m.) > > > Review request for Calligra. > > > Description > ------- > > when you add new style or when you change name of style, patch checks that > the new name for > > style is unique. patch doesnt let you to go another style or close dialog or > apply and ok until the name > > that you have choosen for your style be unique. > > > Diffs > ----- > > plugins/textshape/dialogs/StyleManager.h 44dff97 > plugins/textshape/dialogs/StyleManager.cpp c318dd7 > plugins/textshape/dialogs/CharacterGeneral.h bb72b88 > plugins/textshape/dialogs/CharacterGeneral.cpp 4923087 > plugins/textshape/dialogs/ParagraphGeneral.h 5ef5c86 > plugins/textshape/dialogs/ParagraphGeneral.cpp 8ef5b7f > plugins/textshape/dialogs/StyleManagerDialog.h 56e36b4 > plugins/textshape/dialogs/StyleManagerDialog.cpp d423ae0 > plugins/textshape/dialogs/StylesModel.h 53c0225 > plugins/textshape/dialogs/StylesModel.cpp 3b03f1b > > Diff: http://git.reviewboard.kde.org/r/104882/diff/ > > > Testing > ------- > > it is ok and works right :) > > > Thanks, > > mojtaba shahi > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
