Add support for Terminology escape sequences #16

vinipsmaker · 2018-01-28T04:52:34Z

Updates #12

swsnr · 2018-01-28T10:05:28Z

Thanks, but can you add a few comments to explain what the code does and why terminology needs so much more code than iterm2? How does the terminology format look like?

swsnr · 2018-01-28T10:42:04Z

src/terminal.rs

+pub enum ImageFormat {
+    ITerm2,
+    Terminology,
+}


Nice change :)

Can you add doc comments to the enum and each field, perhaps with a link to the documentation for each of these formats. For iterm2 it's here: https://www.iterm2.com/documentation-images.html

swsnr · 2018-01-28T10:42:43Z

src/terminal.rs

@@ -48,6 +48,8 @@ pub fn columns() -> u16 {
 enum Terminal {
    /// iTerm2
    ITerm2,
+    /// Terminology


Can you perhaps add a link to the Terminology website? I'll add one for the iterm website as well :)

swsnr · 2018-01-28T10:43:53Z

src/commonmark.rs

@@ -440,6 +440,68 @@ impl<'a, W: Write + 'a> Context<'a, W> {
        );
        write!(self.output.writer, "{}", osc(&command))
    }
+
+    /// Write an inline image for Terminology.


I think this method needs much more documentation; I don't even understand what it does at all 🙈

Can you perhaps explain how it works, and also how the image format of terminology looks like?

swsnr · 2018-01-28T10:44:59Z

src/commonmark.rs

+            use std::process::Command;
+            use termion::terminal_size;
+
+            let lines = terminal_size().unwrap_or((0, 24)).1;


Please move this to the terminal module, similar to the existing helpers for the column with.

Also, please don't compute it inline, but add it to the Context. There's already a field for the columns, why not replace it with a size field with a corresponding struct Size?

swsnr · 2018-01-28T10:46:12Z

src/commonmark.rs

+
+            let lines = terminal_size().unwrap_or((0, 24)).1;
+
+            Command::new("identify")


Can't we use a crate here? Perhaps image? I'd prefer if we could work without a runtime dependency on imagemagick here.

If it's not possible, tho, please document that identify must be in $PATH for Terminology image support.

We can use a crate here. But do you want to inflate the binary size with parsing code for multiple image formats in an application that is essentially used in terminals and it'd be a natural solution to depend on external commands?

Just choose an option and I'll go with it. I didn't add a crate because I thought you'd dislike this option.

swsnr · 2018-01-28T10:49:28Z

src/commonmark.rs

+
+                    let o = String::from_utf8(o.stdout).or(Err(ErrorKind::InvalidData))?;
+                    let mut tokens = o.split(' ');
+                    let (w, h) = (get_v(tokens.next())? as f64, get_v(tokens.next())? as f64);


Please .map over tokens to parse each item and use collect to extract the result; this avoids the extra helper function.

swsnr · 2018-01-28T10:49:54Z

src/commonmark.rs

+                            .ok_or(Into::<Error>::into(ErrorKind::InvalidData))
+                    }
+
+                    let o = String::from_utf8(o.stdout).or(Err(ErrorKind::InvalidData))?;


Please do not abbreviate names. Use output instead of o.

swsnr · 2018-01-28T10:50:34Z

src/commonmark.rs

+                .map(str::to_owned)
+                .ok_or(ErrorKind::InvalidData)?)
+        } else {
+            From::from(path.to_str().ok_or(ErrorKind::InvalidData)?)


This else is the same as the first if, isn't it? Please refactor this condition to avoid the duplication.

That said, you don't need to check whether path is relative; joining a path with an absolute path just returns the latter, so you can just use self.input.base_dir.join(path) in all cases, and it'll always give you a correct path.

This else is the same as the first if, isn't it?

You're right. Gonna do.

so you can just use self.input.base_dir.join(path) in all cases, and it'll always give you a correct path

If path is already absolute, I don't want to change the path by joining.

You won't change the path, I think; I prefer having a simpler code path with less conditions.

You won't change the path, I think

You're right: https://play.rust-lang.org/?gist=5651ff43abd5f8bc7af2eaeb1c0cde36&version=stable

But I still need to have different code path for remote URLs.

swsnr · 2018-01-28T10:53:28Z

src/commonmark.rs

+            path.to_str()
+                .map(|p| p.starts_with("http"))
+                .unwrap_or(false)
+        }


Do we need this? In the if below the behaviour doesn't change for HTTP URLs.

If we do need this, please extract into a resources helper module and use Url to implement it, ie, Url::parse(…).map(|url| url.scheme == "https" || url.scheme == "http").unwrap_or(false).

Perhaps you can even generalize it into a is_remote_reference function, as in Url::parse(…).map(|url| url.scheme != "file").unwrap_or(false).

vinipsmaker · 2018-01-28T12:43:40Z

About the comments that I didn't answer explicitly: I won't comment them. I'll just commit the code.

swsnr · 2018-01-28T12:47:19Z

@vinipsmaker Thanks for replying. Just FYI, I did a bit of refactoring which adds the entire terminal size to the Context. If you have a little patience, I can push the code today, so that you can rebase on it and make use of it.

As for the binary size: syntect—which I use for highlighting—already makes the binary huge because it adds dumps of syntax rules for all the languages it supports. Another crate won't make a difference, so I think we should prefer easy of use here: cargo install mdcat should install all that we need, w/o having to install anything else like imagemagick.

vinipsmaker · 2018-01-28T13:19:52Z

Just FYI, I did a bit of refactoring which adds the entire terminal size to the Context. If you have a little patience, I can push the code today, so that you can rebase on it and make use of it.

Okay. Gonna do the other changes locally and wait for your code.

Updates swsnr#12

vinipsmaker · 2018-01-28T19:40:59Z

@lunaryorn, the PR is ready to the new round of review.

swsnr · 2018-01-28T19:46:41Z

@vinipsmaker Thanks 👍 I took a cursory look and I like it a lot more now 👍 I'll take a more detailed look tomorrow, but I think I'll merge it without more review!

vinipsmaker · 2018-01-28T20:23:37Z

I forgot the screenshot:

swsnr · 2018-01-31T17:16:06Z

@vinipsmaker Merged, with some modifications tho to account for some other refactoring of mine.

I also disabled remote images in Terminology for now, because I'm not sure how they work.

For instance what happens when you try to show something that is no image, like when you get a 404 or so? What does Terminology do in this case?

I plan to add remote images to iTerm2 too, and I'll just fetch images with reqwest then; perhaps that's the better way for Terminology, too?

I'm sorry if there's an obvious answer to this question; I just don't how Terminology works, and I don't have access to a system where I can try 😊

swsnr · 2018-01-31T17:16:27Z

PS: I'm sorry that it took me so long to merge it. 😔

vinipsmaker · 2018-01-31T17:36:56Z

I also disabled remote images in Terminology for now, because I'm not sure how they work.

They do work. No special treatment. Just give an absolute link (remote or local) and Terminology will fetch the resource.

For instance what happens when you try to show something that is no image, like when you get a 404 or so? What does Terminology do in this case?

Terminology will asynchronously fetch the resource. You'll see a running circle to know that a resource is being fetched.

If resource fails to load, no problem. You'll just have an empty space there.

I plan to add remote images to iTerm2 too, and I'll just fetch images with reqwest then; perhaps that's the better way for Terminology, too?

It's more inconvenient to have a blocking load of a file. Ideally, Terminology approach would work better. However, because the need to compute a "proportion", it'd be nice to fetch the file ourselves and then feed it to terminology.

Before we do that, first I need to know if we can send files instead file references. References to /tmp are too brittle to even being considered as a solution (my system could just erase these files as soon as mdcat finishes to prevent "DoS"/dangling-references there).

I can see there are fr/fs/fx commands on the Terminology documentation, but I don't know how they work. I can only work on such projects on weekends, so you'll have to wait until I can test anything.

swsnr · 2018-02-02T08:49:52Z

@vinipsmaker Nice, that's cool. I think we should rely on Terminology fetching images then; it's better if it's asynchronous.

I am just a bit concerned that if the image fails to fetch the user is left with an empty space w/o indication what's gone wrong. Currently mdcat shows the image URL for remote images; if we let Terminology download the image, and it fails, it's too late to fallback to showing the image link.

Any idea? Just a minor concern, tho. I'll re-enable remote images for Terminology anyway.

See GH-16

vinipsmaker · 2018-02-03T13:27:30Z

I talked to Terminology/Enlightenment developers over the week. From the discussion, I guess the two best solutions are:

Transform mdcat into mdless.
Rely on Terminology to async load images.

I am just a bit concerned that if the image fails to fetch the user is left with an empty space w/o indication what's gone wrong.

You'll see the spinning circle, but if it fails to load, the spinning circle just disappear, so no indication of failure is left. You'll have a good hint that the resource failed to load, but no notification will stand there.

Any idea? Just a minor concern, tho.

Maybe a --no-fetch-remote CLI arg. This argument will also please users who are concerned with privacy (downloading resources from a server and revealing their IP).

But I can't already work around this w/o any extra code. If I set TERMINOLOGY env to 0, then I can disable extra escape sequences from Terminology.

I'll re-enable remote images for Terminology anyway.

Great!

swsnr · 2018-02-04T14:05:19Z

Transform mdcat into mdless.

You mean with paging? I don't think I'd like to do that; iTerm2 at least is better at paging and scrolling then less. I myself use less only because of lesspipe.sh; I wish it would allow to disable paging and scrolling.

I'll add a --local-only flag to disable remote resources anyway, if only to avoid large downloads over flaky mobile connections.

vinipsmaker · 2018-02-04T14:08:29Z

Okay. Sounds perfect. Best tool to read MarkDown inside the terminal :)

swsnr · 2018-02-04T14:08:57Z

Thanks 😊

vinipsmaker force-pushed the master branch 4 times, most recently from c4f2379 to e54b84c Compare January 28, 2018 05:11

swsnr suggested changes Jan 28, 2018

View reviewed changes

vinipsmaker force-pushed the master branch 5 times, most recently from 1f0f10a to ee1fd7f Compare January 28, 2018 19:20

Add support for Terminology escape sequences

d50a85a

Updates swsnr#12

vinipsmaker force-pushed the master branch from ee1fd7f to d50a85a Compare January 28, 2018 19:23

swsnr closed this in ed1d9de Jan 31, 2018

swsnr added a commit that referenced this pull request Feb 2, 2018

Enable remote images in Terminology again

4f74df8

See GH-16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Terminology escape sequences #16

Add support for Terminology escape sequences #16

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 28, 2018

swsnr Jan 28, 2018

swsnr Jan 28, 2018

swsnr Jan 28, 2018

swsnr Jan 28, 2018

swsnr Jan 28, 2018

vinipsmaker Jan 28, 2018

swsnr Jan 28, 2018

swsnr Jan 28, 2018

swsnr Jan 28, 2018

swsnr Jan 28, 2018

vinipsmaker Jan 28, 2018

swsnr Jan 28, 2018

vinipsmaker Jan 28, 2018

swsnr Jan 28, 2018

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 28, 2018

vinipsmaker commented Jan 28, 2018

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 28, 2018

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 31, 2018

swsnr commented Jan 31, 2018 •

edited

Loading

vinipsmaker commented Jan 31, 2018

swsnr commented Feb 2, 2018

vinipsmaker commented Feb 3, 2018

swsnr commented Feb 4, 2018

vinipsmaker commented Feb 4, 2018

swsnr commented Feb 4, 2018


		let lines = terminal_size().unwrap_or((0, 24)).1;

		Command::new("identify")

Add support for Terminology escape sequences #16

Add support for Terminology escape sequences #16

Conversation

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 28, 2018

vinipsmaker commented Jan 28, 2018

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 28, 2018

vinipsmaker commented Jan 28, 2018

swsnr commented Jan 31, 2018

swsnr commented Jan 31, 2018 • edited Loading

vinipsmaker commented Jan 31, 2018

swsnr commented Feb 2, 2018

vinipsmaker commented Feb 3, 2018

swsnr commented Feb 4, 2018

vinipsmaker commented Feb 4, 2018

swsnr commented Feb 4, 2018

swsnr commented Jan 31, 2018 •

edited

Loading