WIP - Added prompt user for url #17062

jklepatch · 2017-05-27T11:43:44Z

This is not ready yet, I would just like to have some feedback.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #17047
These changes do not require tests because _____

This change is

highfive · 2017-05-27T11:43:50Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

paulrouget

It's weird. The changes in Cargo.lock make it impossible for me to compile.

./mach build -d --verbose
cargo build -v
error: failed to parse lock file at: /Users/paul/git/servo/Cargo.lock

Caused by:
  package `gcc 0.3.45 (registry+https://github.com/rust-lang/crates.io-index)` is specified as a dependency, but is missing from the package list

I'll see later what's going on.

paulrouget · 2017-05-27T12:05:31Z

ports/glutin/window.rs

+                        sanitized_url = sanitize_url(user_input).unwrap();
+                        //Maybe need to change WindowEvent::LoadUrl() to accept a ServoUrl instead of a String?
+                        self.event_queue.borrow_mut().push(WindowEvent::LoadUrl(sanitized_url.to_string()));
+                    }


You can simplify all of this with a if let Some(…) expression. No match needed, an no need to have this intermediate user_input value.

Let me know if you need help here.

paulrouget · 2017-05-27T12:05:31Z

ports/glutin/window.rs

@@ -1402,6 +1424,10 @@ fn is_printable(key_code: VirtualKeyCode) -> bool {
    }
 }

+fn sanitize_url(url: String) -> Result<ServoUrl, ()> {
+    Ok(ServoUrl::parse(url.trim()).unwrap())


sanitize_url should fail if ServoUrl::parse fails. Don't return Ok(…). See how to use Result::map_err https://doc.rust-lang.org/std/result/enum.Result.html#method.map_err.

Maybe rename sanitize_url to sanitize_and_parse_url .

FYI, unwrap() on a failing Result will panic. See https://is.gd/RAdLog

paulrouget · 2017-05-27T12:05:31Z

ports/glutin/window.rs

@@ -1268,12 +1269,33 @@ impl WindowMethods for Window {
                    self.event_queue.borrow_mut().push(WindowEvent::Reload);
                }
            }
+            //On windows 8.1 when pressing ctrl + anything, ch = Some('\u{unicode_value}')


We need to investigate that before merging this PR.

paulrouget · 2017-05-27T12:34:50Z

ports/glutin/window.rs

+                if let Some(true) = PREFS.get("shell.builtin-key-shortcuts.enabled").as_boolean() {
+                    let user_input: String;
+                    //TODO: 
+                    // 1) make it work when clicking on "validate" button - Currently only works when pressing enter


Maybe it has to do with the fact we're blocking the main thread.
On Mac, the buttons work, but we get the beatchball.

Maybe this code should run in a different thread.

paulrouget · 2017-05-29T06:57:35Z

ports/glutin/window.rs

@@ -1402,6 +1419,10 @@ fn is_printable(key_code: VirtualKeyCode) -> bool {
    }
 }

+fn sanitize_and_parse_url(url: String) -> Result<ServoUrl, ()> {


Actually, we might want to use ParseError for now instead of an empty error.

paulrouget · 2017-05-29T06:58:55Z

ports/glutin/window.rs

+                    // 1) make it work when clicking on "validate" button - Currently only works when pressing enter
+                    // 2) Erratic behavior - sometime the dialog reloads empty after pressing enter
+                    // (windows 8.1)
+                    if let Some(user_input) = tinyfiledialogs::input_box("Enter a URL", "URL:", "")  {


The default value should be the current url, not "".

I'm wondering if we have any locales support. I'm not sure we want to keep english sentences in the code.

paulrouget · 2017-05-29T07:41:28Z

@jdm is tinyfiledialogs thread safe? It appears to work well here (osx) if I open a prompt from a different thread. But I'm not sure it's the right thing to do.

jdm · 2017-05-29T12:34:51Z

I do not believe the library does anything special to ensure that it works from non-main threads. I suspect that at least one platform will not work as intended if used that way.


        # This is a combination of 2 commits.

# The first commit's message is: Added prompt user for url Fix Cargo.lock corrupted problem Refactored event handler for CTRL + L Made sanitize_and_parse_url() handle correctly parse errors # This is the commit message #2: Made the prompt default to the current url


        Added prompt user for url

Fix Cargo.lock corrupted problem Refactored event handler for CTRL + L Made sanitize_and_parse_url() handle correctly parse errors Made the prompt default to the current url Made sanitize_and_parse_url() return ParseError instead of empty error

jklepatch · 2017-06-04T06:02:23Z

Currently, in handle_key() (window.rs) I am matching ctrl + L on Key instead of ch. This is a temporary solution. Once this related issue is solved, I can match on ch and the PR will be ready.

bors-servo · 2017-06-14T15:34:06Z

☔ The latest upstream changes (presumably #17184) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-11-15T22:34:49Z

Closing due to inactivity.

highfive assigned pcwalton May 27, 2017

highfive added the S-awaiting-review label May 27, 2017

jklepatch changed the title ~~Added prompt user for url~~ WIP - Added prompt user for url May 27, 2017

paulrouget reviewed May 27, 2017

View changes

jklepatch force-pushed the jklepatch:prompt_user_for_url branch from a5eb57e to 010a9a0 May 28, 2017

paulrouget reviewed May 29, 2017

View changes

paulrouget assigned paulrouget and unassigned pcwalton May 30, 2017

jklepatch added 2 commits May 27, 2017

jklepatch force-pushed the jklepatch:prompt_user_for_url branch from 010a9a0 to cf174f1 Jun 4, 2017

highfive added the S-needs-rebase label Jun 14, 2017

jdm closed this Nov 15, 2017

atouchet mentioned this pull request Mar 5, 2018

Prompt URL on Cmd/Ctrl-L #20182

Merged

3 of 5 tasks complete

atouchet mentioned this pull request Jun 29, 2018

Workaround Windows issue with URL prompt #21103

Closed

0 of 5 tasks complete

servo / servo

WIP - Added prompt user for url #17062

WIP - Added prompt user for url #17062

jklepatch commented May 27, 2017 •

edited by larsbergstrom

highfive commented May 27, 2017

paulrouget left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget commented May 29, 2017

jdm commented May 29, 2017

jklepatch commented Jun 4, 2017 •

edited

bors-servo commented Jun 14, 2017

jdm commented Nov 15, 2017

servo / servo

Join GitHub today

WIP - Added prompt user for url #17062

WIP - Added prompt user for url #17062

Conversation

jklepatch commented May 27, 2017 • edited by larsbergstrom

highfive commented May 27, 2017

paulrouget left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

paulrouget commented May 29, 2017

jdm commented May 29, 2017

jklepatch commented Jun 4, 2017 • edited

bors-servo commented Jun 14, 2017

jdm commented Nov 15, 2017

jklepatch commented May 27, 2017 •

edited by larsbergstrom

jklepatch commented Jun 4, 2017 •

edited