'Re: Review Request: FreOffice Viewer to FreOffice Editor'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: FreOffice Viewer to  FreOffice Editor
From:       "Amit Aggarwal" <amitcs06 () gmail ! com>
Date:       2010-06-20 6:54:15
Message-ID: 20100620065415.6758.30326 () localhost
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4386/#review6194
-----------------------------------------------------------

Please find my some comments

AboutDialog.h
Line 47: Where are you using hbl ?

CMakeLists.txt
Line19: X11_X11_LIB get deleted. Is it not required ?

Line 30-32: share/fre/.template - fre is not proper name. Can you look some other \
name ?

Common.h
Line 69: Naming convention is not consistent with other.

Why Line 71 ppsx is moved to 89

Line 115: Please delete the commented code if not required. 

MainWindow.ui
Line 126: actionCopy_2 can we rename it without _2 ?

Line 141: why this ../../ is added ? Please delete it from all places. 
Line 150,156,168,177,183,189,219,228,234,246,258,270,282,294,306,etc : Same comment

Line 368: why ":" after <normaloff> ?
Same for everywhere 

MainWindow.h 

Line 82: Delete extra spaces.
Delete extra include header files if not require ? Or use foward Declaration.
Line 88: Naming convetion not consistent. 

Line 116: Same comment.

Line 323,324: Why  two different function Cant we close one doc or multiple docs in \
same slot.

MainWindow.cpp
Line 296: Same Why ": " is required ?

Line 296- 339: Why not make one function and pass Icon name as parameter As its doing \
same thing everytime.

Line 390: Declare int i in for loop or initalized that variable. 
Line 391- 395 :Why this for loop is required ?

Line 396- 410 : USe the same function which mentioned above. 

Line 441: Why check of kwview is required ? 
Line 455: Same comment.

- Amit

On 2010-06-19 08:26:44, pratik vyas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4386/
> -----------------------------------------------------------
> 
> (Updated 2010-06-19 08:26:44)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> We have provided following functionality in freoffice viewer to make editing \
> enable. 
> Description:-
> 
> Basic Editing :-
> 	we have extended the functionality by adding the editing feature 
> 	in FreOffice. Now the user can open a new Text Document and write text in 
> 	it or he/she can open and edit document. It provides more usability to the 
> 	user as now he can make changes in the document on move. User Interface 
> 	is designed keeping in view that user gets required tool in minimum number 
> 	of clicks. 
> 
> 1.UI Development includes following :-
> 	      Edit ToolBar
> 	      Format Options Frame
> 	      Font Options Frame
> 	      Main Menu Modification
> 
> 2. New Functionality added in main menu
> Save 
> SaveAs
> Close
> 
> 3. Following are the Edit Tool Bar options that we have enable for word document : 
> 	 Bold 
> 	 Italic 
> 	 Underline 
> 	 Cut 
> 	 Copy 
> 	 Paste 
> 	 Undo
> 	 Redo
> 	 Insert Options:- 
> 	 Font Style 
> 	 Font Size 
> 	 Text Color 
> 	 Subscript 
> 	 Superscript 
> 
> 	 Format Options:- 
> 	         Bold 
> 	         Italic 
> 	         Underline 
> 	         Align left 
> 	         Align Right 
> 	         Align Center 
> Align Justify 
> 	         Bullets 
> 	         Numbers 
> 
> 4  In Presenter Part user can edit existing presention and save it in odp or ppt \
> format  
> Here i have attached patch ,please kindly review the new changes.
> 
> 
> This addresses bugs Editor, FreOffice, Viewer, and to.
> https://bugs.kde.org/show_bug.cgi?id=Editor
> https://bugs.kde.org/show_bug.cgi?id=FreOffice
> https://bugs.kde.org/show_bug.cgi?id=Viewer
> https://bugs.kde.org/show_bug.cgi?id=to
> 
> 
> Diffs
> -----
> 
> /trunk/koffice/tools/f-office/AboutDialog.h 1138588 
> /trunk/koffice/tools/f-office/AboutDialog.cpp 1138588 
> /trunk/koffice/tools/f-office/CMakeLists.txt 1138588 
> /trunk/koffice/tools/f-office/Common.h 1138588 
> /trunk/koffice/tools/f-office/FreOffice.desktop 1138588 
> /trunk/koffice/tools/f-office/FreOffice.qrc 1138588 
> /trunk/koffice/tools/f-office/HildonMenu.cpp 1138588 
> /trunk/koffice/tools/f-office/MainWindow.h 1138588 
> /trunk/koffice/tools/f-office/MainWindow.cpp 1138588 
> /trunk/koffice/tools/f-office/MainWindow.ui 1138588 
> /trunk/koffice/tools/f-office/Splash.h 1138588 
> /trunk/koffice/tools/f-office/Splash.cpp 1138588 
> 
> Diff: http://reviewboard.kde.org/r/4386/diff
> 
> 
> Testing
> -------
> 
> We have check functionality all use cases that is newly added 
> Testing for memory leaks has been done by valgrind
> 
> 
> Thanks,
> 
> pratik
> 
> 

[Attachment #5 (text/html)]

<html>
 <body>
  <div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
   <table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;">  <tr>
     <td>
      This is an automatically generated e-mail. To reply, visit:
      <a href="http://reviewboard.kde.org/r/4386/">http://reviewboard.kde.org/r/4386/</a>
  </td>
    </tr>
   </table>
   <br />

 <pre>Please find my some comments