Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor tty_rows, tty_cols #4117

Closed
JeffBezanson opened this issue Aug 20, 2013 · 26 comments
Closed

refactor tty_rows, tty_cols #4117

JeffBezanson opened this issue Aug 20, 2013 · 26 comments

Comments

@JeffBezanson
Copy link
Member

With the display subsystem in place, these functions don't really make sense or work correctly any more. There should be a way for Displays to indicate their size, and types that implement show and/or writemime to determine this size if they want to truncate their output.

We could add a displaysize function that works similarly to display. Displays can overload it, and calling it as displaysize([mimetype]) will tell you the size. Maybe there could be some way to get the equivalent of showall using something like with_display_size.

@StefanKarpinski
Copy link
Member

Using infinite (or typemax(Int)) display size would be a reasonable to indicate that you want showall.

@JeffBezanson
Copy link
Member Author

Yes. But the awkwardness comes from the fact that usually the Display controls what the size is, but in that case the user wants to control it.

@ivarne
Copy link
Member

ivarne commented Aug 21, 2013

Nice that others are thinking about this issue also. I tried to raise the issue in #3932 (comment), but @stevengj did not approve then. Maybe because he was focused on images. I have thought of different solutions to give this hint to the writemime function, but my only suggestion is to use a Dict{String,Int} and document the interpretation of some possible keys ( like c_cols/c_rows, p_hight/p_widht, max_size_mb). The same dictionary can be used as an optional argument to display so that the user is able to override the default size parameters from the display driver.

@stevengj
Copy link
Member

The key is to attach this information to the correct abstraction.

The thing is, the writemime function does not and cannot know what Display it is going to, so a displaysize function is useless here. It may not be going to any display at all — the user may just be writing to a file. The whole point of writemime/display is to separate the writing of information in a given format from how it is displayed.

@stevengj
Copy link
Member

I tend to think that the number of rows/cols to display for truncated "text/plain" representations of data should simply be a global preference (possibly set by the current TTY device, but more likely not), independent of any particular Display.

@StefanKarpinski
Copy link
Member

So you don't want it to be responsive to the size of the display? That seems like a step backwards.

@stevengj
Copy link
Member

First of all, if you want the global preference to be responsive to the size of the TTY, you can do that, just don't tie it to the Display mechanism. The latter is what doesn't make sense to me.

Second, it is not clear that one wants it to be responsive to the size of the display (at least, not by default). When I make my terminal window bigger, what makes you think that means I want to see more rows of matrices? Maybe I would rather see more of the command history instead. And if I make my terminal window only 5 lines high, what makes you think that means I only want to see 5 lines of matrices? Maybe I still want to see 30 lines, and just don't mind scrolling.

@ivarne
Copy link
Member

ivarne commented Aug 21, 2013

writemime does not and should not know what Display is going to do. writemime only cares what Display wants to get. writemime know and must know what MIME type to produce. The problem is that writemime might have many ways of generating the appropriate mime type (different resolutions and truncation rules, pdf versions and so on), and sometimes the user who calls ´display(obj)´ might want to change the default options. That might be accomplished by adding fields to the type of obj, but that clutters obj, and increases memory usage if you have millions of instances of obj.

PS: Responsive to the size of display is much more useful for the width than the height of the terminal. Horizontal scrolling is usually not that pleasant.

@stevengj
Copy link
Member

@ivarne, you are missing the point. I have no problem with some (generally type-dependent) setting for how writemime works. But these preferences cannot be attached to Display because writemime is not only not passed a Display, but it may not even be called from a Display at all. They can be global preferences, or attached to individual instances of the type, depending on what makes sense in particular cases. In the case of how many rows/columns of matrices to display, I think a global preference (I suppose defaulting to the size of the TTY) is sensible.

Note that mismatching the horizontal size is only really a pain in a terminal. In a browser display like IJulia, scrolling is potentially not as problematic, and it is perfectly sensible to show a fixed portion of a matrix (e.g. fitting into 30x80 characters) independent of the size of the browser window.

@stevengj
Copy link
Member

Right now, if the user (or a module like IJulia) wants to change the truncation size, it can do so: it just sets ENV["COLUMNS"] and ENV["LINES"]. That doesn't seem so bad to me.

stevengj added a commit to JuliaLang/IJulia.jl that referenced this issue Aug 21, 2013
@timholy
Copy link
Member

timholy commented Aug 22, 2013

Profile.print() adjusts its display to the number of columns. It uses indentation to indicate nesting, and if the nesting gets deeper than is feasible given the terminal width, it starts prepending +14 in front to indicate that it would have liked to indent another 14 spaces. The issue is, parsing that is much more of a "cognitive load" than scrolling past a bunch of lines where indentation is indicated visually.

Hence, I agree with others that it's nice to be able to adjust output to the display size.

@stevengj
Copy link
Member

If a display(d::MyDisplay) wants to change the terminal-size setting temporarily, it can already do so:

function display(d::MyDisplay)
    rows, cols = tty_rows(), tty_cols()
    try
          ENV["LINES"] = myrows
          ENV["COLUMNS"] = mycolumns
          .... call writemime functions ...
    finally
          ENV["LINES"] = rows
          ENV["COLUMNS"] = cols
    end
end

We could make this a bit easier via a with_tty_size(rows, cols) do .... end function, but in general a global preference seems right to me for this sort of thing, which can be changed in a dynamically scoped way if needed. The reason it should be a global preference is that if the user or a module like IJulia wants to change the default number of lines printed, then one wants to be able to do so globally rather than for each Display or display call.

@kmsquire
Copy link
Member

Second, it is not clear that one wants it to be responsive to the size of the display (at least, not by default). When I make my terminal window bigger, what makes you think that means I want to see more rows of matrices? Maybe I would rather see more of the command history instead. And if I make my terminal window only 5 lines high, what makes you think that means I only want to see 5 lines of matrices? Maybe I still want to see 30 lines, and just don't mind scrolling.

Just to add my 2 cents... Pandas on python does a rather nice job of formatting DataFrames for display, but I find it frustrating that it can't figure out my display size and formats everything for 80 columns until I tell it otherwise. Ditto with Matlab and matrix display (at least older versions--I haven't used it recently).

For me, at least, I would prefer my matrices (and similar objects) to actually display using the full extent of the terminal that it's running in.

@StefanKarpinski
Copy link
Member

Yes, I feel pretty strongly that using the current display size is a much better user experience. I really like the way the Julia repl shows you only as much of a matrix as will fit in your window (not surprising since I implemented that behavior). Doing a fixed-size output would be a major regression in user experience in my view.

@ivarne
Copy link
Member

ivarne commented Aug 27, 2013

As I am apparently missing the point, I have not commented more on this thread for a week. I still feel that there is an obvious solution to this problem that has not been discussed and that @stevengj still has not convinced me that is stupid. It does involve API changes that should be done before 0.2.

The signatures can be extended to take an optional list of named arguments, so that they write writemime(same_as_before ;opts...) and display(same_as_before ;opts...). The documentation should have a list of commonly used arguments, so that the option for adjusting the number of characters a line can have has a consistent name between writemime methods. writemime must also provide default values for the options it supports. The options should be passed directly from the display function to the writemime function. But a MyDisplay <: Display implementation might decide to add more options (like number_of_characters_per_line), if it knows the width of the terminal and the user did not specify character_width when he called display().

I tried to write some draft documentation and it is available at https://gist.github.com/ivarne/a04426ff124962cf64d0

@ivarne
Copy link
Member

ivarne commented Sep 25, 2013

Bump!

My suggestion involves an API change for display and writemime that will be hard to make backward compatible after 0.2 is released, thus I feel that it deserves to be discussed/rejected now rather than later.

I think character rows and cols is one part of a more general problem of how to enable options when converting data to a MIME standard representation. It would be better to have a general way of supplying these options instead of different configuration functions for each module and option.

@timholy
Copy link
Member

timholy commented Sep 25, 2013

Agreed that the client either needs to be able to control or learn what the display setting are (and preferably both). An alternative to the keyword arguments would be to do something like this (apologies for not taking the time to see how the display code is actually implemented, maybe this already exists):

D = getdisplay(io, mimetype, obj)

and then you can query/set D, using all the goodness of multiple dispatch.

Of course base writemime on top of getdisplay so that you can be sure that it's using the same D (no surprises).

@timholy
Copy link
Member

timholy commented Sep 25, 2013

I also agree with @ivarne that this is pretty important. Where possible it should be bidirectional. For example, consider plotting an enormous image in a small amount of screen space, which means it has to be downsampled. There is little point transmitting all that pixel data over a slow network connection, might as well downsample at the source of the data.

@ivarne
Copy link
Member

ivarne commented Sep 25, 2013

It is very hard to reduce the resolution on bitmap graphs if it contains dots or lines that only extend to one pixel. A general algorithm won't recognize those very important features, and some lines and points will just disappear.

@timholy
Copy link
Member

timholy commented Sep 25, 2013

Yes. Any display software has to solve the same problem (monitors have only so many pixels), and there's no universally-acceptable answer. Being able to control the algorithm used to do that is another reason to encourage this effort.

@stevengj
Copy link
Member

My biggest concern with this, aside from the question of defining useful metadata, is that every single writemime method then has to accept keywords, even if it is ignoring them, which is a bit ugly.

One alternative would be to have a writemime_metadata(io, mimetype, data; kws...) function with a default definition = writemime(io, mimetype data). Similarly for a display_metadata. If you want to use the metadata for your datatype, you overload the writemime_metadata function, otherwise you overload writemime. This would have the advantage of backwards compatibility (so no need for this to block 0.2) and of not requiring writemime to deal with keywords in the common case where they are ignored.

Yes, these are awful names, but I'm sure better could be devised; if @JeffBezanson implements dispatch based on the presence of keywords, they can even be called writemime and display.

@nolta
Copy link
Member

nolta commented Nov 20, 2013

MIME types have optional parameters, e.g., text/plain; encoding=UTF-8. So how about just adding parameters to our mime types, e.g.,

function display(io::IO, m::MIME"image/png", x)
    w = m[:width]
    h = m[:height]
    ...

@stevengj
Copy link
Member

@nolta, that sounds like a very interesting idea, and I like the fact that it could potentially be implemented transparently without changing the dispatch signatures.

@ivarne
Copy link
Member

ivarne commented Nov 20, 2013

What is different from what I proposed 3 months, and how will I get access to the m/x dictionary in my writemime implementation?

EDIT: Sorry, I did not understand. I'll wait for the rest of the discussion.

@StefanKarpinski
Copy link
Member

If we're going to have MIME types in Base Julia, I think they should faithfully reproduce the features of mimetypes – a type (parameter), a subtype (also a parameter), and zero or more options, stored in a dict. That way you can dispatch on the type and the subtype and access the options.

@JeffBezanson
Copy link
Member Author

Closing in favor of #5709; we need a better way to deal with printing parameters like output limiting.

simonster added a commit that referenced this issue Jun 19, 2014
Ref #7297

We really need a better solution for this generally (see #4117/#5709)
but I suppose reinstating `ENV` as fallback is easy for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants