Re: [Okular-devel] Review Request 108614: Open url in browser

[Commit Hook]

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

(Updated March 26, 2013, 9:49 p.m.)


Status
--

This change has been marked as submitted.


Review request for Okular.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs
-

  tests/CMakeLists.txt 6a26b3e 
  tests/urldetecttest.cpp PRE-CREATION 
  ui/pageview.cpp e8d481d 
  ui/url_utils.h PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Commit Hook]

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


This review has been submitted with commit 
1121cff1b8600079909424a90923f75b5c3630ad by Albert Astals Cid on behalf of 
Jaydeep Solanki to branch master.

- Commit Hook


On March 25, 2013, 7:51 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 25, 2013, 7:51 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   tests/CMakeLists.txt 6a26b3e 
>   tests/urldetecttest.cpp PRE-CREATION 
>   ui/pageview.cpp e8d481d 
>   ui/url_utils.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 25, 2013, 7:51 p.m.)


Review request for Okular.


Changes
---

moved the prepending of http to getUrl method.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  tests/CMakeLists.txt 6a26b3e 
  tests/urldetecttest.cpp PRE-CREATION 
  ui/pageview.cpp e8d481d 
  ui/url_utils.h PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 25, 2013, 6:56 p.m.)


Review request for Okular.


Changes
---

updated


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  tests/CMakeLists.txt 6a26b3e 
  tests/urldetecttest.cpp PRE-CREATION 
  ui/pageview.cpp e8d481d 
  ui/url_utils.h PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

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


Does KRun know how to run www.google.com ? Doesn't seem to work here, maybe you 
should prepend http:// if there is no protocol in the url?

- Albert Astals Cid


On March 15, 2013, 7:28 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 15, 2013, 7:28 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   tests/CMakeLists.txt 6a26b3e 
>   tests/urldetecttest.cpp PRE-CREATION 
>   ui/pageview.cpp e8d481d 
>   ui/url_utils.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 15, 2013, 7:28 p.m.)


Review request for Okular.


Changes
---

no specific reason.
here's the diff with those urls added.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  tests/CMakeLists.txt 6a26b3e 
  tests/urldetecttest.cpp PRE-CREATION 
  ui/pageview.cpp e8d481d 
  ui/url_utils.h PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

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



tests/urldetecttest.cpp


Your pdf in http://goo.gl/wBM6p had some more "complex" urls, any reason 
you didn't add them here?


- Albert Astals Cid


On March 13, 2013, 2:38 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 13, 2013, 2:38 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   tests/CMakeLists.txt 6a26b3e 
>   tests/urldetecttest.cpp PRE-CREATION 
>   ui/pageview.cpp b018dfe 
>   ui/url_utils.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 13, 2013, 2:38 p.m.)


Review request for Okular.


Changes
---

Sorry, the previous one had a typo.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  tests/CMakeLists.txt 6a26b3e 
  tests/urldetecttest.cpp PRE-CREATION 
  ui/pageview.cpp b018dfe 
  ui/url_utils.h PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 13, 2013, 2:28 p.m.)


Review request for Okular.


Changes
---

a unit test added.
BTW, can I add that banner( that is on top of every file containing copyright 
), with my name ?


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  ui/pageview.cpp b018dfe 
  tests/urldetecttest.cpp PRE-CREATION 
  tests/CMakeLists.txt 6a26b3e 
  ui/url_utils.h PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Azat Khuzhin]

> On March 10, 2013, 12:27 a.m., Azat Khuzhin wrote:
> > Seems more appropriate for me. But one more thing, is reg1 still needed?
> 
> Jaydeep Solanki wrote:
> Yes, because it prevents "asdfhttp://www.google.com"; from getting 
> detected.
> 
> Azat Khuzhin wrote:
> I think that for this it is not necessary.
> Because of "\b" in you first regexp.
> 
> Jaydeep Solanki wrote:
> No.
> '\b' detects word boundry.
> For example, in "asdfhttp://www.google.com";, '/' is not considered as 
> part of a word, so the first 'w' in 'www.' will be considered as boundary, 
> thus detecting 'www.google.com' as a url, which should not be detected.
> A better way to do this is lookbehind assertion, but Qt doesn't support 
> it, so I finally decided to use two regexs.

Yes, it make sense.

BTW qt5 support it https://bugreports.qt-project.org/browse/QTBUG-2371.
Maybe you want to leave a TODO for qt5?


- Azat


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


On March 9, 2013, 8:13 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 9, 2013, 8:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Azat Khuzhin]

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


Seems more appropriate for me. But one more thing, is reg1 still needed?

- Azat Khuzhin


On March 9, 2013, 8:13 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 9, 2013, 8:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Azat Khuzhin]

> On March 10, 2013, 12:27 a.m., Azat Khuzhin wrote:
> > Seems more appropriate for me. But one more thing, is reg1 still needed?
> 
> Jaydeep Solanki wrote:
> Yes, because it prevents "asdfhttp://www.google.com"; from getting 
> detected.

I think that for this it is not necessary.
Because of "\b" in you first regexp.


- Azat


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


On March 9, 2013, 8:13 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 9, 2013, 8:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

> On March 10, 2013, 12:27 a.m., Azat Khuzhin wrote:
> > Seems more appropriate for me. But one more thing, is reg1 still needed?
> 
> Jaydeep Solanki wrote:
> Yes, because it prevents "asdfhttp://www.google.com"; from getting 
> detected.
> 
> Azat Khuzhin wrote:
> I think that for this it is not necessary.
> Because of "\b" in you first regexp.
> 
> Jaydeep Solanki wrote:
> No.
> '\b' detects word boundry.
> For example, in "asdfhttp://www.google.com";, '/' is not considered as 
> part of a word, so the first 'w' in 'www.' will be considered as boundary, 
> thus detecting 'www.google.com' as a url, which should not be detected.
> A better way to do this is lookbehind assertion, but Qt doesn't support 
> it, so I finally decided to use two regexs.
> 
> Azat Khuzhin wrote:
> Yes, it make sense.
> 
> BTW qt5 support it https://bugreports.qt-project.org/browse/QTBUG-2371.
> Maybe you want to leave a TODO for qt5?
> 
> Jaydeep Solanki wrote:
> Oh, I didn't know that, thanks!
> I'll make sure, this gets improved when kdelibs move to qt5.

Do you think you could implement a unittest (just for the 'static QString 
getUrl' method) like the ones we have in the tests/ folder? Have you ever used 
QtTest? If not drop by #okular and ping me.


- Albert


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


On March 9, 2013, 8:13 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 9, 2013, 8:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On March 10, 2013, 12:27 a.m., Azat Khuzhin wrote:
> > Seems more appropriate for me. But one more thing, is reg1 still needed?
> 
> Jaydeep Solanki wrote:
> Yes, because it prevents "asdfhttp://www.google.com"; from getting 
> detected.
> 
> Azat Khuzhin wrote:
> I think that for this it is not necessary.
> Because of "\b" in you first regexp.
> 
> Jaydeep Solanki wrote:
> No.
> '\b' detects word boundry.
> For example, in "asdfhttp://www.google.com";, '/' is not considered as 
> part of a word, so the first 'w' in 'www.' will be considered as boundary, 
> thus detecting 'www.google.com' as a url, which should not be detected.
> A better way to do this is lookbehind assertion, but Qt doesn't support 
> it, so I finally decided to use two regexs.
> 
> Azat Khuzhin wrote:
> Yes, it make sense.
> 
> BTW qt5 support it https://bugreports.qt-project.org/browse/QTBUG-2371.
> Maybe you want to leave a TODO for qt5?

Oh, I didn't know that, thanks!
I'll make sure, this gets improved when kdelibs move to qt5.


- Jaydeep


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


On March 9, 2013, 8:13 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 9, 2013, 8:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On March 10, 2013, 12:27 a.m., Azat Khuzhin wrote:
> > Seems more appropriate for me. But one more thing, is reg1 still needed?
> 
> Jaydeep Solanki wrote:
> Yes, because it prevents "asdfhttp://www.google.com"; from getting 
> detected.
> 
> Azat Khuzhin wrote:
> I think that for this it is not necessary.
> Because of "\b" in you first regexp.

No.
'\b' detects word boundry.
For example, in "asdfhttp://www.google.com";, '/' is not considered as part of a 
word, so the first 'w' in 'www.' will be considered as boundary, thus detecting 
'www.google.com' as a url, which should not be detected.
A better way to do this is lookbehind assertion, but Qt doesn't support it, so 
I finally decided to use two regexs.


- Jaydeep


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


On March 9, 2013, 8:13 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 9, 2013, 8:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On March 10, 2013, 12:27 a.m., Azat Khuzhin wrote:
> > Seems more appropriate for me. But one more thing, is reg1 still needed?

Yes, because it prevents "asdfhttp://www.google.com"; from getting detected.


- Jaydeep


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


On March 9, 2013, 8:13 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 9, 2013, 8:13 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 9, 2013, 8:13 p.m.)


Review request for Okular.


Changes
---

How about this?


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  ui/pageview.cpp b018dfe 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Azat Khuzhin]

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


It trims up to ascii characters.
Example: "http://google.com/ф"; -> "http://google.com/";

- Azat Khuzhin


On March 5, 2013, 1:26 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 5, 2013, 1:26 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 5, 2013, 1:26 p.m.)


Review request for Okular.


Changes
---

this should fix it.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  ui/pageview.cpp b018dfe 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Azat Khuzhin]

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


"http://google.com)" is not a valid URL.
Because host/port can't contain ")" and path must start from "/" see 
http://www.ietf.org/rfc/rfc3986.txt

Also I have a question, why you are using regular expression for this, I don't 
think that it is easy to write such regexp.
Why you don't just split by "[\t\n ]" and validate URL using qt/kde method? (I 
think qt/kde must have such)

But if there is no such, here is a good example of regexp for URL's - 
http://stackoverflow.com/a/190405
Also see note about "u" modifier.

- Azat Khuzhin


On March 2, 2013, 10:39 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 2, 2013, 10:39 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Azat Khuzhin]

> On March 3, 2013, 4:47 p.m., Albert Astals Cid wrote:
> > This is still detecting "http://google.com)" as something you can call open 
> > link on. I'm not sure if "http://google.com)" is valid or not, but when you 
> > click on the "Go got 'http://google.com)'" I am getting a "Malformed URL" 
> > error. 
> > 
> > Can you please investigate if 'http://google.com)' is supposed to be a 
> > correct URL or not? If it is maybe you can try to find out why the 
> > "Malformed URL" error happens?

"http://google.com)" is not a valid URL.
Because host/port can't contain ")" and path must start from "/" see 
http://www.ietf.org/rfc/rfc3986.txt

Also I have a question, why you are using regular expression for this, I don't 
think that it is easy to write such regexp.
Why you don't just split by "[\t\n ]" and validate URL using qt/kde method? (I 
think qt/kde must have such)

But if there is no such, here is a good example of regexp for URL's - 
http://stackoverflow.com/a/190405
Also see note about "u" modifier.


- Azat


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


On March 2, 2013, 10:39 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 2, 2013, 10:39 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

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


This is still detecting "http://google.com)" as something you can call open 
link on. I'm not sure if "http://google.com)" is valid or not, but when you 
click on the "Go got 'http://google.com)'" I am getting a "Malformed URL" 
error. 

Can you please investigate if 'http://google.com)' is supposed to be a correct 
URL or not? If it is maybe you can try to find out why the "Malformed URL" 
error happens?

- Albert Astals Cid


On March 2, 2013, 10:39 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated March 2, 2013, 10:39 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> pdf with links update : http://goo.gl/wBM6p
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp b018dfe 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 2, 2013, 10:39 p.m.)


Review request for Okular.


Description (updated)
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.

pdf with links update : http://goo.gl/wBM6p


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs
-

  ui/pageview.cpp b018dfe 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated March 2, 2013, 10:33 p.m.)


Review request for Okular.


Changes
---

I don't know why but after seeing the regex used by konversation I was 
convinced that it is a good idea, but now I thing it isn't.
Anyways, not detecting "xyz.com" will make it less error prone.
Updated.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  ui/pageview.cpp b018dfe 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

> On March 1, 2013, 6:33 p.m., Albert Astals Cid wrote:
> > Why the exceptions? Looks a bit weird to me.
> 
> Jaydeep Solanki wrote:
> Please elaborate.

Why would you want to detect google.com) as a url?


- Albert


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


On Feb. 24, 2013, 1:55 a.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 24, 2013, 1:55 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On March 1, 2013, 6:33 p.m., Albert Astals Cid wrote:
> > Why the exceptions? Looks a bit weird to me.

Please elaborate.


- Jaydeep


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


On Feb. 24, 2013, 1:55 a.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 24, 2013, 1:55 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

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


Why the exceptions? Looks a bit weird to me.

- Albert Astals Cid


On Feb. 24, 2013, 1:55 a.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 24, 2013, 1:55 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On Feb. 21, 2013, 3:29 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2185
> > 
> >
> > This regexp still needs some tweaking, right now if i pass 
> > "holahttps://okular.org"; it returns https://okular.org that in my opinion 
> > is not correct. You should enforce spaces (or newlines or tabs) around the 
> > regexp to make sure the url is a single word. Also it would be cool if you 
> > could write down all the urls you've used to positively and negatively test 
> > the regexp so we can put it in a testcase
> 
> Jaydeep Solanki wrote:
> You mean "holahttps://okular.org"; should not be detected as an url, & 
> "hola https://okular.org"; should be.
> Am I right ??
> 
> Albert Astals Cid wrote:
> Exactly
> 
> Mailson Menezes wrote:
> Try to have a look on how konversation extracts url from strings. More 
> precisely the function extractUrlData in common.cpp

thanks, it helped a lot.


- Jaydeep


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


On Feb. 24, 2013, 1:55 a.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 24, 2013, 1:55 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated Feb. 24, 2013, 1:55 a.m.)


Review request for Okular.


Changes
---

regex updated,
pdf containing test cases : http://goo.gl/wBM6p


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  ui/pageview.cpp 60a273d 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Mailson Menezes]

> On Feb. 21, 2013, 3:29 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2185
> > 
> >
> > This regexp still needs some tweaking, right now if i pass 
> > "holahttps://okular.org"; it returns https://okular.org that in my opinion 
> > is not correct. You should enforce spaces (or newlines or tabs) around the 
> > regexp to make sure the url is a single word. Also it would be cool if you 
> > could write down all the urls you've used to positively and negatively test 
> > the regexp so we can put it in a testcase
> 
> Jaydeep Solanki wrote:
> You mean "holahttps://okular.org"; should not be detected as an url, & 
> "hola https://okular.org"; should be.
> Am I right ??
> 
> Albert Astals Cid wrote:
> Exactly

Try to have a look on how konversation extracts url from strings. More 
precisely the function extractUrlData in common.cpp


- Mailson


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


On Feb. 20, 2013, 2:21 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 20, 2013, 2:21 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Mailson Menezes]

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


- Mailson Menezes


On Feb. 20, 2013, 2:21 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 20, 2013, 2:21 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

> On Feb. 21, 2013, 3:29 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2185
> > 
> >
> > This regexp still needs some tweaking, right now if i pass 
> > "holahttps://okular.org"; it returns https://okular.org that in my opinion 
> > is not correct. You should enforce spaces (or newlines or tabs) around the 
> > regexp to make sure the url is a single word. Also it would be cool if you 
> > could write down all the urls you've used to positively and negatively test 
> > the regexp so we can put it in a testcase
> 
> Jaydeep Solanki wrote:
> You mean "holahttps://okular.org"; should not be detected as an url, & 
> "hola https://okular.org"; should be.
> Am I right ??

Exactly


- Albert


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


On Feb. 20, 2013, 2:21 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 20, 2013, 2:21 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On Feb. 21, 2013, 3:29 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2183
> > 
> >
> > Mere looks, please put the static on front

oops!


> On Feb. 21, 2013, 3:29 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2185
> > 
> >
> > This regexp still needs some tweaking, right now if i pass 
> > "holahttps://okular.org"; it returns https://okular.org that in my opinion 
> > is not correct. You should enforce spaces (or newlines or tabs) around the 
> > regexp to make sure the url is a single word. Also it would be cool if you 
> > could write down all the urls you've used to positively and negatively test 
> > the regexp so we can put it in a testcase

You mean "holahttps://okular.org"; should not be detected as an url, & "hola 
https://okular.org"; should be.
Am I right ??


- Jaydeep


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


On Feb. 20, 2013, 2:21 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 20, 2013, 2:21 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

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



ui/pageview.cpp


Mere looks, please put the static on front



ui/pageview.cpp


This regexp still needs some tweaking, right now if i pass 
"holahttps://okular.org"; it returns https://okular.org that in my opinion is 
not correct. You should enforce spaces (or newlines or tabs) around the regexp 
to make sure the url is a single word. Also it would be cool if you could write 
down all the urls you've used to positively and negatively test the regexp so 
we can put it in a testcase



ui/pageview.cpp


return QString() not ""


- Albert Astals Cid


On Feb. 20, 2013, 2:21 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 20, 2013, 2:21 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated Feb. 20, 2013, 2:21 p.m.)


Review request for Okular.


Changes
---

icon removed.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  ui/pageview.cpp 60a273d 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

> On Jan. 27, 2013, 7:02 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2775
> > 
> >
> > Why did you decide that icon name?
> 
> Jaydeep Solanki wrote:
> I had two options for goto icon, "go_goto" & "go_goto_page" but none of 
> them shows an icon.
> I decided to keep, the later one because I have used it in one of my 
> apps, & at that time it worked.
> 
> Albert Astals Cid wrote:
> Use "kdialog --geticon foo", then select again the Actions combo (seems 
> that's a bug) and once you are there try to look for an icon that makes 
> sense, if not, i guess it's better to simply use no icon than one that 
> doesn't exist
> 
> Jaydeep Solanki wrote:
> Don't you think that the 'search for' option that appears when right 
> clicked on selected text, should have 'edit-web-search' icon,
> & go to 'link' should have 'preferences-web-browser-shortcuts'.
> I just had a look at the icons & I think 'edit-web-search' describes the 
> search action more appropriately.

I agree these icons look "good" in the current theme I am using but the names 
seem to say they are though for other actions, meaning in other theme or in the 
future they may chage and not be useful for us at all. I think i'd go with no 
icons for now.


- Albert


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


On Feb. 17, 2013, 7:04 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 17, 2013, 7:04 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On Jan. 27, 2013, 7:02 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2775
> > 
> >
> > Why did you decide that icon name?
> 
> Jaydeep Solanki wrote:
> I had two options for goto icon, "go_goto" & "go_goto_page" but none of 
> them shows an icon.
> I decided to keep, the later one because I have used it in one of my 
> apps, & at that time it worked.
> 
> Albert Astals Cid wrote:
> Use "kdialog --geticon foo", then select again the Actions combo (seems 
> that's a bug) and once you are there try to look for an icon that makes 
> sense, if not, i guess it's better to simply use no icon than one that 
> doesn't exist

Don't you think that the 'search for' option that appears when right clicked on 
selected text, should have 'edit-web-search' icon,
& go to 'link' should have 'preferences-web-browser-shortcuts'.
I just had a look at the icons & I think 'edit-web-search' describes the search 
action more appropriately.


- Jaydeep


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


On Feb. 17, 2013, 7:04 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 17, 2013, 7:04 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

> On Jan. 27, 2013, 7:02 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2775
> > 
> >
> > Why did you decide that icon name?
> 
> Jaydeep Solanki wrote:
> I had two options for goto icon, "go_goto" & "go_goto_page" but none of 
> them shows an icon.
> I decided to keep, the later one because I have used it in one of my 
> apps, & at that time it worked.

Use "kdialog --geticon foo", then select again the Actions combo (seems that's 
a bug) and once you are there try to look for an icon that makes sense, if not, 
i guess it's better to simply use no icon than one that doesn't exist


- Albert


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


On Feb. 17, 2013, 7:04 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 17, 2013, 7:04 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

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



ui/pageview.cpp


Please do not return 0 as a QString, that's just evil



ui/pageview.cpp


isEmpty is "bigger" than isNull, so just check for empty is enough


- Albert Astals Cid


On Feb. 17, 2013, 7:04 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 17, 2013, 7:04 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

> On Jan. 27, 2013, 7:02 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2184
> > 
> >
> > Would this detect 
> > 
> > hellohttp://foo.comlala
> > 
> > As a link?
> > 
> > Also did you create the regex yourself or it comes from somewhere?

this is not mine, but the updated one is mine.


> On Jan. 27, 2013, 7:02 p.m., Albert Astals Cid wrote:
> > ui/pageview.cpp, line 2775
> > 
> >
> > Why did you decide that icon name?

I had two options for goto icon, "go_goto" & "go_goto_page" but none of them 
shows an icon.
I decided to keep, the later one because I have used it in one of my apps, & at 
that time it worked.


- Jaydeep


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


On Feb. 17, 2013, 7:04 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Feb. 17, 2013, 7:04 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Jaydeep Solanki]

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

(Updated Feb. 17, 2013, 7:04 p.m.)


Review request for Okular.


Changes
---

fixed all, but the icon problem.


Description
---

When selected text is right clicked, it checks for a url, if found, a QAction 
is added to open the url in the default browser.


This addresses bug 281027.
http://bugs.kde.org/show_bug.cgi?id=281027


Diffs (updated)
-

  ui/pageview.cpp 60a273d 

Diff: http://git.reviewboard.kde.org/r/108614/diff/


Testing
---

just check if the icon appears properly, because I have an issue with the icon.


Thanks,

Jaydeep Solanki

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel

Re: [Okular-devel] Review Request 108614: Open url in browser

[Albert Astals Cid]

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



ui/pageview.cpp


Would this detect 

hellohttp://foo.comlala

As a link?

Also did you create the regex yourself or it comes from somewhere?



ui/pageview.cpp


declare the url as const



ui/pageview.cpp


comparing a QString against 0 is bad



ui/pageview.cpp


Why did you decide that icon name?



ui/pageview.cpp


I'd prefer if you used "new KRun(url)" here, it's supposed to be more KDE 
integrated


- Albert Astals Cid


On Jan. 27, 2013, 2:25 p.m., Jaydeep Solanki wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108614/
> ---
> 
> (Updated Jan. 27, 2013, 2:25 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Description
> ---
> 
> When selected text is right clicked, it checks for a url, if found, a QAction 
> is added to open the url in the default browser.
> 
> 
> This addresses bug 281027.
> http://bugs.kde.org/show_bug.cgi?id=281027
> 
> 
> Diffs
> -
> 
>   ui/pageview.cpp 60a273d 
> 
> Diff: http://git.reviewboard.kde.org/r/108614/diff/
> 
> 
> Testing
> ---
> 
> just check if the icon appears properly, because I have an issue with the 
> icon.
> 
> 
> Thanks,
> 
> Jaydeep Solanki
> 
>

___
Okular-devel mailing list
Okular-devel@kde.org
https://mail.kde.org/mailman/listinfo/okular-devel