Review Request 109746: Expansion of the References section by adding features to insert hyperlinks, bookmarks and links to bookmarks.

[Aman Madaan]

> On April 1, 2013, 4:48 p.m., Thorsten Zachmann wrote:
> > libs/kotext/KoTextEditor.cpp, line 1448
> > <http://git.reviewboard.kde.org/r/109746/diff/7/?file=126911#file126911line1448>
> >
> >     Would it be possible to use a static QRegExp or a membervariable here as regenerating a QRegExp all the time is quite expensive.

Fixed this as well as a similar code in LinkInsertionDialog.cpp


> On April 1, 2013, 4:48 p.m., Thorsten Zachmann wrote:
> > plugins/textshape/ReferencesTool.h, line 93
> > <http://git.reviewboard.kde.org/r/109746/diff/7/?file=126913#file126913line93>
> >
> >     can this method made const?

I don't think so because we call non const functions in it.


> On April 1, 2013, 4:48 p.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/LinkInsertionDialog.cpp, line 179
> > <http://git.reviewboard.kde.org/r/109746/diff/7/?file=126916#file126916line179>
> >
> >     please remove the blank before the )

The function was deleted.


> On April 1, 2013, 4:48 p.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/ManageBookmarkDialog.cpp, line 25
> > <http://git.reviewboard.kde.org/r/109746/diff/7/?file=126920#file126920line25>
> >
> >     Using a static here seems to be wrong to me.

Since this is a global variable that won't possibly be used in any other file, adding static is actually a good thing if I am not wrong.
(This is not my code) 


> On April 1, 2013, 4:48 p.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/ManageBookmarkDialog.cpp, line 85
> > <http://git.reviewboard.kde.org/r/109746/diff/7/?file=126920#file126920line85>
> >
> >     The test for isNull seem wrong to me as it will break the loop without setting a name. However if the nsma is empty it does not. Removing it should be enough.

Indeed. Since empty input can never be accepted (default behavior) as you have noted in another issue, the isEmpty() check is redundant as well. I have tried to improve it.


> On April 1, 2013, 4:48 p.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/SimpleLinksWidget.h, line 50
> > <http://git.reviewboard.kde.org/r/109746/diff/7/?file=126921#file126921line50>
> >
> >     I think this should be change to a QList<QString>.

This was deleted altogether.

---

Thanks for the review.


> On April 1, 2013, 4:48 p.m., Thorsten Zachmann wrote:
> > plugins/textshape/dialogs/ManageBookmarkDialog.cpp, lines 125-131
> > <http://git.reviewboard.kde.org/r/109746/diff/7/?file=126920#file126920line125>
> >
> >     This allows adding of a bookmark with an empty name. However when changing the name a empty name is not allowed which is inconsistent.

Fixed by using a similar approach to rename.


- Aman


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


On March 31, 2013, 4:17 a.m., Aman Madaan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109746/
> -----------------------------------------------------------
> 
> (Updated March 31, 2013, 4:17 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> This patch adds the following features : 
> 
> 1. Inserting hyperlinks
>    -- User has a choice of inserting a link by specifying the target and the link text. Additionally, the 
>       user may supply values for target and rel attributes using a drop down list.  
>    -- The user may fetch the title from the web page itself (contents of the "title" tag ). This feature also
>       handles URL redirects. For example, ieee.com is finally redirected to http://www.ieee.org/index.html.
>       This feature will especially help in cases when a user has a list of links referred to and a list of 
>       references has to be created. Just copy/pasting the URL, clicking fetch and then insert will do the job.
> 
> 2. Linking to bookmarks
>    -- A user can specify a bookmark name and the link text. To help the user with inserting bookmark, 
>       an auto completer is used. This becomes helpful when the bookmarks have been given a name that 
>       are related to the context. (lastpagefirstpara or conclusions).
> 
> 3. Adding Bookmark using a labeled widget ( similar to the way footnote and endnote labels are entered )
> 
> 
> Diffs
> -----
> 
>   libs/kotext/KoTextEditor.h f96ab70 
>   libs/kotext/KoTextEditor.cpp 07bcad8 
>   plugins/textshape/CMakeLists.txt b2bf9ae 
>   plugins/textshape/ReferencesTool.h 21caea8 
>   plugins/textshape/ReferencesTool.cpp 46bf06f 
>   plugins/textshape/dialogs/LinkInsertionDialog.h PRE-CREATION 
>   plugins/textshape/dialogs/LinkInsertionDialog.cpp PRE-CREATION 
>   plugins/textshape/dialogs/LinkInsertionDialog.ui PRE-CREATION 
>   plugins/textshape/dialogs/ManageBookmark.ui PRE-CREATION 
>   plugins/textshape/dialogs/ManageBookmarkDialog.h PRE-CREATION 
>   plugins/textshape/dialogs/ManageBookmarkDialog.cpp PRE-CREATION 
>   plugins/textshape/dialogs/SimpleLinksWidget.h PRE-CREATION 
>   plugins/textshape/dialogs/SimpleLinksWidget.cpp PRE-CREATION 
>   plugins/textshape/dialogs/SimpleLinksWidget.ui PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/109746/diff/
> 
> 
> Testing
> -------
> 
> I have tested the following components : 
> 	
> 1	  Inserting Hyperlinks			
> 1.1	Simplest case  (works)		
> 1.2	Fetch title from URL (works)		
> 1.3	Fetch title from URL which has re directions (works)		
> 1.4	specify a URL without a scheme (works, appending an http:// to all the schemeless urls)
> 1.5	fetching from an non existing URL (Time out occurs and notifies user about the same)
> 				
> 2	Adding Bookmarks			
> 2.1	Simplest case (works)		
> 2.2	Adding a duplicate bookmark i.e. using a name that has been used before(User is notified and bookmark addition is aborted)	
> 				
> 3	Adding link to a bookmark			
> 3.1	Simplest case (works)		
> 3.2	Adding a link to a non existing bookmark (works,User is notified about non existence of the bookmark)
> 				
> 4	Manage bookmarks
> 4.1	user clicks on manage bookmarks	(works, Copy pasted the existing implementation. Looks like delete bookmark is broken.)	
> 
> 
> Thanks,
> 
> Aman Madaan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130401/c4f3f111/attachment.htm>