Merge lp://staging/~diogenesethecynical/lernid/Bug793793 into lp://staging/lernid

Proposed by Diogenese TheCynical
Status: Rejected
Rejected by: John S. Gruber
Proposed branch: lp://staging/~diogenesethecynical/lernid/Bug793793
Merge into: lp://staging/lernid
Diff against target: 12 lines (+2/-0)
1 file modified
lernid/widgets/Browser.py (+2/-0)
To merge this branch: bzr merge lp://staging/~diogenesethecynical/lernid/Bug793793
Reviewer Review Type Date Requested Status
John S. Gruber Pending
Review via email: mp+66785@code.staging.launchpad.net

Description of the change

Fixing bug #793793. Minor change in Browser.py.

To post a comment you must log in.
Revision history for this message
John S. Gruber (jsjgruber) wrote :

Thanks for the patch! I tested it and it worked for me. I'll review it for merging for a release in August or a little later. (I'm testing a new version in my lernid-proposed ppa now.)

Revision history for this message
Mohammad AbuShady (coalwater) wrote :

I've read the patch and I have a small remark about it, the code says if the URL doesn't start with http:// or https:// assume it's http://
But assume I wanted to share a link that starts with ftp:// or something like that, O know it's unlikely but if we assume such link was shared, the code will still prepend it with http:// so the final URL would be "http://ftp://example.com" .. just felt i should point this case out, just wanted to make sure you still want this patch assuming no links with different protocols would be shared.

Revision history for this message
John S. Gruber (jsjgruber) wrote :

This isn't for sharing a url, however, it is a shortcut for someone using the Open Url... I don't think the browser will work with ftp:// ,etc., at this point anyway, but I guess it could in the future.

Diogenese, you could check have urlparse do some work here and have it find the scheme. If it finds no scheme you could do the prepend of http:// . That might make the code more robust. I'm not sure this is terribly important.

Revision history for this message
Diogenese TheCynical (diogenesethecynical) wrote :

> This isn't for sharing a url, however, it is a shortcut for someone using the
> Open Url... I don't think the browser will work with ftp:// ,etc., at this
> point anyway, but I guess it could in the future.
>
> Diogenese, you could check have urlparse do some work here and have it find
> the scheme. If it finds no scheme you could do the prepend of http:// . That
> might make the code more robust. I'm not sure this is terribly important.

Hi John

You are absolutely right. We might need to support ftp:// in future.
What do you think about this:

if URL starts with www then preappend http:// to it.

If this is not what you had in mind when you said find the scheme, please do let me know.

Regards.

Revision history for this message
John S. Gruber (jsjgruber) wrote :

Thanks for the comment, Diiogenese. That's not what I had in mind, and
I think it would be a bad idea, frankly.

I had in mind the possibility of using the python urlparse function
(as in import urlparse), and have it parse the
user's input, to see if it parses a scheme, the scheme is the http: or
ftp: part of the url. If it doesn't then you
could append http:// to what the student asked for.

When programming in python I usually have a browser tab open to the
python library documentation, and when
programming in lernid I usually have another tab open to look at the
pygtk documentation. You should be able
to google for these. Another useful thing is the following: if looking
for a particular item, such as the string "label"
you can use fgrep -ir label * when sitting in the lernid top
directory to search the whole project for the string "label" (or
"LaBel", the -i is for case-insensitive and the -r option is the
recursive option. I also often will open a separate terminal
window and run python in it. Then I might say "import urlparse" and
then help(urlparse) or dir(urlparse) to see what's in the module. I
might then run it through some examples in that window until I think I
have good code to put into lernid. It can
save time. You might already do these things, of course, or have
better ways of working.

Some urls don't contain www. -- that's just a computer naming
convention. I often use just http://launchpad.net or
http://gmail.com, myself. The http: or https: is what makes something
a web url, and the SOMETHING: at the head
of the URL is the scheme, the protocol to be used to retrieve the
information the URL specifies.

Thanks.

John

On Sun, Jul 24, 2011 at 10:14 PM, Diogenese TheCynical
<email address hidden> wrote:
>> This isn't for sharing a url, however, it is a shortcut for someone using the
>> Open Url... I don't think the browser will work with ftp:// ,etc., at this
>> point anyway, but I guess it could in the future.
>>
>> Diogenese, you could check have urlparse do some work here and have it find
>> the scheme. If it finds no scheme you could do the prepend of http:// . That
>> might make the code more robust. I'm not sure this is terribly important.
>
> Hi John
>
> You are absolutely right. We might need to support ftp:// in future.
> What do you think about this:
>
> if URL starts with www then preappend http:// to it.
>
> If this is not what you had in mind when you said find the scheme, please do let me know.
>
> Regards.
> --
> https://code.launchpad.net/~diogenesethecynical/lernid/Bug793793/+merge/66785
> You are requested to review the proposed merge of lp:~diogenesethecynical/lernid/Bug793793 into lp:lernid.
>

Revision history for this message
Diogenese TheCynical (diogenesethecynical) wrote :
Download full text (3.9 KiB)

Hi John

Sorry for my late reply. I understood what you are trying to say. I have made the appropriate changes. Before making these changes I thought I will update to the latest code and after that, I cannot launch Lernid . I got error (ImportError: No module named gtkmozembed)

I am looking into how to solve it (quick search revealed install python-gnome2-extras, but that did not solve this , I am looking into it however. Probem should be of my environment if others can work with the same code.

But I thought I will fix the code and get it reviewed by you before asking for merge. Here is what I plan to do:

from urlparse import urlparse

def set_location(self, url):
        o = urlparse(url)
        if o.scheme == '':
            url += 'http://'

Please let me know your thoughts on this. Once you give a go, I will formally ask you to merge.

Thanks for all your help .

Regards.

________________________________
From: John S. Gruber <email address hidden>
To: <email address hidden>
Sent: Monday, July 25, 2011 10:57 AM
Subject: Re: [Merge] lp:~diogenesethecynical/lernid/Bug793793 into lp:lernid

Thanks for the comment, Diiogenese. That's not what I had in mind, and
I think it would be a bad idea, frankly.

I had in mind the possibility of using the python urlparse function
(as in import urlparse), and have it parse the
user's input, to see if it parses a scheme, the scheme is the http: or
ftp: part of the url. If it doesn't then you
could append http:// to what the student asked for.

When programming in python I usually have a browser tab open to the
python library documentation, and when
programming in lernid I usually have another tab open to look at the
pygtk documentation. You should be able
to google for these. Another useful thing is the following: if looking
for a particular item, such as the string "label"
you can use fgrep -ir label *  when sitting in the lernid top
directory to search the whole project for the string "label" (or
"LaBel", the -i is for case-insensitive and the -r option is the
recursive option. I also often will open a separate terminal
window and run python in it. Then I might say "import urlparse" and
then help(urlparse) or dir(urlparse) to see what's in the module. I
might then run it through some examples in that window until I think I
have good code to put into lernid. It can
save time. You might already do these things, of course, or have
better ways of working.

Some urls don't contain www.  -- that's just a computer naming
convention. I often use just http://launchpad.net or
http://gmail.com, myself. The http: or https: is what makes something
a web url, and the SOMETHING: at the head
of the URL is the scheme, the protocol to be used to retrieve the
information the URL specifies.

Thanks.

John

On Sun, Jul 24, 2011 at 10:14 PM, Diogenese TheCynical
<email address hidden> wrote:
>> This isn't for sharing a url, however, it is a shortcut for someone using the
>> Open Url... I don't think the browser will work with ftp:// ,etc., at this
>> point anyway, but I guess it could in the future.
>>
>> Diogenese, you could check have urlparse do some work here and have it find
>> the scheme. If it f...

Read more...

Revision history for this message
John S. Gruber (jsjgruber) wrote :

I suspect the code you are working on is back-level. There should be no import of gtkmozembed anymore. You may want to grab lp:lernid with bzr branch lp:lernid again. The latest revision should be numbered 228 and tagged as 0.8.1.5 . If you go to the top lernid directory and do fgrep -r gtkmozembed you should only get one hit, for the debian/changelog entry for the change that eliminated mention of gtkmozembed (thanks to Chris Coulson).

You do understand me about using urlparse--I'd only suggest url = "http://" + url so the order of the two items is correct. I do think using urlparse is more flexible.

>>> url='www.me.com'
>>> url
'www.me.com'
>>> url += 'http://'
>>> url
'www.me.comhttp://'
>>>

Revision history for this message
John S. Gruber (jsjgruber) wrote :

Clearing as Diogenese submitted a better substitute.

Unmerged revisions

226. By Diogenese <email address hidden>

Bug 793793

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches