Auto merge of #45187 - GuillaumeGomez:doc-ui-improvement, r=QuietMisdreavus

Improve sidebar rendering and add methods list

I suppose it can be reviewed as is, but this is just the first step of a more global plan.

cc @rust-lang/docs @nical

And a screenshot of course:

<img width="1440" alt="screen shot 2017-10-10 at 23 38 45" src="https://user-images.githubusercontent.com/3050060/31412170-657beaf6-ae14-11e7-9f01-1e562a034595.png">

Author: bors

src/librustdoc/html/render.rs

@@ -37,7 +37,7 @@ pub use self::ExternalLocation::*;
 use std::ascii::AsciiExt;
 use std::cell::RefCell;
 use std::cmp::Ordering;
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashSet};
 use std::default::Default;
 use std::error;
 use std::fmt::{self, Display, Formatter, Write as FmtWrite};
@@ -3207,12 +3207,37 @@ fn render_deref_methods(w: &mut fmt::Formatter, cx: &Context, impl_: &Impl,
     }
 }
 
+fn should_render_item(item: &clean::Item, deref_mut_: bool) -> bool {
+    let self_type_opt = match item.inner {
+        clean::MethodItem(ref method) => method.decl.self_type(),
+        clean::TyMethodItem(ref method) => method.decl.self_type(),
+        _ => None
+    };
+
+    if let Some(self_ty) = self_type_opt {
+        let (by_mut_ref, by_box) = match self_ty {
+            SelfTy::SelfBorrowed(_, mutability) |
+            SelfTy::SelfExplicit(clean::BorrowedRef { mutability, .. }) => {
+                (mutability == Mutability::Mutable, false)
+            },
+            SelfTy::SelfExplicit(clean::ResolvedPath { did, .. }) => {
+                (false, Some(did) == cache().owned_box_did)
+            },
+            _ => (false, false),
+        };
+
+        (deref_mut_ || !by_mut_ref) && !by_box
+    } else {
+        false
+    }
+}
+
 fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLink,
                render_mode: RenderMode, outer_version: Option<&str>,
                show_def_docs: bool) -> fmt::Result {
     if render_mode == RenderMode::Normal {
         let id = derive_id(match i.inner_impl().trait_ {
-            Some(ref t) => format!("impl-{}", Escape(&format!("{:#}", t))),
+            Some(ref t) => format!("impl-{}", small_url_encode(&format!("{:#}", t))),
             None => "impl".to_string(),
         });
         write!(w, "<h3 id='{}' class='impl'><span class='in-band'><code>{}</code>",
@@ -3244,30 +3269,7 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi
 
         let render_method_item: bool = match render_mode {
             RenderMode::Normal => true,
-            RenderMode::ForDeref { mut_: deref_mut_ } => {
-                let self_type_opt = match item.inner {
-                    clean::MethodItem(ref method) => method.decl.self_type(),
-                    clean::TyMethodItem(ref method) => method.decl.self_type(),
-                    _ => None
-                };
-
-                if let Some(self_ty) = self_type_opt {
-                    let (by_mut_ref, by_box) = match self_ty {
-                        SelfTy::SelfBorrowed(_, mutability) |
-                        SelfTy::SelfExplicit(clean::BorrowedRef { mutability, .. }) => {
-                            (mutability == Mutability::Mutable, false)
-                        },
-                        SelfTy::SelfExplicit(clean::ResolvedPath { did, .. }) => {
-                            (false, Some(did) == cache().owned_box_did)
-                        },
-                        _ => (false, false),
-                    };
-
-                    (deref_mut_ || !by_mut_ref) && !by_box
-                } else {
-                    false
-                }
-            },
+            RenderMode::ForDeref { mut_: deref_mut_ } => should_render_item(&item, deref_mut_),
         };
 
         match item.inner {
@@ -3514,12 +3516,48 @@ impl<'a> fmt::Display for Sidebar<'a> {
     }
 }
 
+fn get_methods(i: &clean::Impl, for_deref: bool) -> Vec<String> {
+    i.items.iter().filter_map(|item| {
+        match item.name {
+            // Maybe check with clean::Visibility::Public as well?
+            Some(ref name) if !name.is_empty() && item.visibility.is_some() && item.is_method() => {
+                if !for_deref || should_render_item(item, false) {
+                    Some(format!("<a href=\"#method.{name}\">{name}</a>", name = name))
+                } else {
+                    None
+                }
+            }
+            _ => None,
+        }
+    }).collect::<Vec<_>>()
+}
+
+// The point is to url encode any potential character from a type with genericity.
+fn small_url_encode(s: &str) -> String {
+    s.replace("<", "%3C")
+     .replace(">", "%3E")
+     .replace(" ", "%20")
+     .replace("?", "%3F")
+     .replace("'", "%27")
+     .replace("&", "%26")
+     .replace(",", "%2C")
+     .replace(":", "%3A")
+     .replace(";", "%3B")
+     .replace("[", "%5B")
+     .replace("]", "%5D")
+}
+
 fn sidebar_assoc_items(it: &clean::Item) -> String {
     let mut out = String::new();
     let c = cache();
     if let Some(v) = c.impls.get(&it.def_id) {
-        if v.iter().any(|i| i.inner_impl().trait_.is_none()) {
-            out.push_str("<li><a href=\"#methods\">Methods</a></li>");
+        let ret = v.iter()
+                   .filter(|i| i.inner_impl().trait_.is_none())
+                   .flat_map(|i| get_methods(i.inner_impl(), false))
+                   .collect::<String>();
+        if !ret.is_empty() {
+            out.push_str(&format!("<a class=\"sidebar-title\" href=\"#methods\">Methods\
+                                   </a><div class=\"sidebar-links\">{}</div>", ret));
         }
 
         if v.iter().any(|i| i.inner_impl().trait_.is_some()) {
@@ -3535,16 +3573,40 @@ fn sidebar_assoc_items(it: &clean::Item) -> String {
                     let inner_impl = target.def_id().or(target.primitive_type().and_then(|prim| {
                         c.primitive_locations.get(&prim).cloned()
                     })).and_then(|did| c.impls.get(&did));
-                    if inner_impl.is_some() {
-                        out.push_str("<li><a href=\"#deref-methods\">");
+                    if let Some(impls) = inner_impl {
+                        out.push_str("<a class=\"sidebar-title\" href=\"#deref-methods\">");
                         out.push_str(&format!("Methods from {:#}&lt;Target={:#}&gt;",
-                                                  impl_.inner_impl().trait_.as_ref().unwrap(),
-                                                  target));
-                        out.push_str("</a></li>");
+                                              impl_.inner_impl().trait_.as_ref().unwrap(),
+                                              target));
+                        out.push_str("</a>");
+                        let ret = impls.iter()
+                                       .filter(|i| i.inner_impl().trait_.is_none())
+                                       .flat_map(|i| get_methods(i.inner_impl(), true))
+                                       .collect::<String>();
+                        out.push_str(&format!("<div class=\"sidebar-links\">{}</div>", ret));
                     }
                 }
             }
-            out.push_str("<li><a href=\"#implementations\">Trait Implementations</a></li>");
+            let mut links = HashSet::new();
+            let ret = v.iter()
+                       .filter_map(|i| if let Some(ref i) = i.inner_impl().trait_ {
+                           let out = format!("{:#}", i).replace("<", "&lt;").replace(">", "&gt;");
+                           let encoded = small_url_encode(&format!("{:#}", i));
+                           let generated = format!("<a href=\"#impl-{}\">{}</a>", encoded, out);
+                           if !links.contains(&generated) && links.insert(generated.clone()) {
+                               Some(generated)
+                           } else {
+                               None
+                           }
+                       } else {
+                           None
+                       })
+                       .collect::<String>();
+            if !ret.is_empty() {
+                out.push_str("<a class=\"sidebar-title\" href=\"#implementations\">\
+                              Trait Implementations</a>");
+                out.push_str(&format!("<div class=\"sidebar-links\">{}</div>", ret));
+            }
         }
     }
 
@@ -3565,7 +3627,7 @@ fn sidebar_struct(fmt: &mut fmt::Formatter, it: &clean::Item,
     sidebar.push_str(&sidebar_assoc_items(it));
 
     if !sidebar.is_empty() {
-        write!(fmt, "<div class=\"block items\"><ul>{}</ul></div>", sidebar)?;
+        write!(fmt, "<div class=\"block items\">{}</div>", sidebar)?;
     }
     Ok(())
 }
@@ -3592,8 +3654,6 @@ fn sidebar_trait(fmt: &mut fmt::Formatter, it: &clean::Item,
         sidebar.push_str("<li><a href=\"#provided-methods\">Provided Methods</a></li>");
     }
 
-    sidebar.push_str(&sidebar_assoc_items(it));
-
     let c = cache();
 
     if let Some(implementors) = c.implementors.get(&it.def_id) {
@@ -3607,15 +3667,17 @@ fn sidebar_trait(fmt: &mut fmt::Formatter, it: &clean::Item,
 
     sidebar.push_str("<li><a href=\"#implementors\">Implementors</a></li>");
 
-    write!(fmt, "<div class=\"block items\"><ul>{}</ul></div>", sidebar)
+    sidebar.push_str(&sidebar_assoc_items(it));
+
+    write!(fmt, "<div class=\"block items\">{}</div>", sidebar)
 }
 
 fn sidebar_primitive(fmt: &mut fmt::Formatter, it: &clean::Item,
                      _p: &clean::PrimitiveType) -> fmt::Result {
     let sidebar = sidebar_assoc_items(it);
 
     if !sidebar.is_empty() {
-        write!(fmt, "<div class=\"block items\"><ul>{}</ul></div>", sidebar)?;
+        write!(fmt, "<div class=\"block items\">{}</div>", sidebar)?;
     }
     Ok(())
 }
@@ -3625,7 +3687,7 @@ fn sidebar_typedef(fmt: &mut fmt::Formatter, it: &clean::Item,
     let sidebar = sidebar_assoc_items(it);
 
     if !sidebar.is_empty() {
-        write!(fmt, "<div class=\"block items\"><ul>{}</ul></div>", sidebar)?;
+        write!(fmt, "<div class=\"block items\">{}</div>", sidebar)?;
     }
     Ok(())
 }
@@ -3642,7 +3704,7 @@ fn sidebar_union(fmt: &mut fmt::Formatter, it: &clean::Item,
     sidebar.push_str(&sidebar_assoc_items(it));
 
     if !sidebar.is_empty() {
-        write!(fmt, "<div class=\"block items\"><ul>{}</ul></div>", sidebar)?;
+        write!(fmt, "<div class=\"block items\">{}</div>", sidebar)?;
     }
     Ok(())
 }
@@ -3658,7 +3720,7 @@ fn sidebar_enum(fmt: &mut fmt::Formatter, it: &clean::Item,
     sidebar.push_str(&sidebar_assoc_items(it));
 
     if !sidebar.is_empty() {
-        write!(fmt, "<div class=\"block items\"><ul>{}</ul></div>", sidebar)?;
+        write!(fmt, "<div class=\"block items\">{}</div>", sidebar)?;
     }
     Ok(())
 }

src/librustdoc/html/static/rustdoc.css

@@ -188,18 +188,16 @@ nav.sub {
 
 .js-only, .hidden { display: none !important; }
 
-.sidebar {
-	padding: 10px;
-}
 .sidebar img {
 	margin: 20px auto;
 	display: block;
+	margin-top: 10px;
 }
 
 .sidebar .location {
 	border: 1px solid;
 	font-size: 17px;
-	margin: 30px 0 20px 0;
+	margin: 30px 10px 20px 10px;
 	text-align: center;
 	word-wrap: break-word;
 }
@@ -220,7 +218,7 @@ nav.sub {
 .location a:first-child { font-weight: 500; }
 
 .block {
-	padding: 0 10px;
+	padding: 0;
 	margin-bottom: 14px;
 }
 .block h2, .block h3 {
@@ -229,7 +227,7 @@ nav.sub {
 	text-align: center;
 }
 .block ul, .block li {
-	margin: 0;
+	margin: 0 10px;
 	padding: 0;
 	list-style: none;
 }
@@ -245,6 +243,23 @@ nav.sub {
 	transition: border 500ms ease-out;
 }
 
+.sidebar-title {
+	border-top: 1px solid #777;
+	border-bottom: 1px solid #777;
+	text-align: center;
+	font-size: 17px;
+	margin-bottom: 5px;
+}
+
+.sidebar-links {
+	margin-bottom: 15px;
+}
+
+.sidebar-links > a {
+	padding-left: 10px;
+	width: 100%;
+}
+
 .content {
 	padding: 15px 0;
 }

src/tools/linkchecker/main.rs

@@ -69,15 +69,32 @@ struct FileEntry {
 
 type Cache = HashMap<PathBuf, FileEntry>;
 
+fn small_url_encode(s: &str) -> String {
+    s.replace("<", "%3C")
+     .replace(">", "%3E")
+     .replace(" ", "%20")
+     .replace("?", "%3F")
+     .replace("'", "%27")
+     .replace("&", "%26")
+     .replace(",", "%2C")
+     .replace(":", "%3A")
+     .replace(";", "%3B")
+     .replace("[", "%5B")
+     .replace("]", "%5D")
+}
+
 impl FileEntry {
     fn parse_ids(&mut self, file: &Path, contents: &str, errors: &mut bool) {
         if self.ids.is_empty() {
             with_attrs_in_source(contents, " id", |fragment, i, _| {
                 let frag = fragment.trim_left_matches("#").to_owned();
+                let encoded = small_url_encode(&frag);
                 if !self.ids.insert(frag) {
                     *errors = true;
                     println!("{}:{}: id is not unique: `{}`", file.display(), i, fragment);
                 }
+                // Just in case, we also add the encoded id.
+                self.ids.insert(encoded);
             });
         }
     }