-
Notifications
You must be signed in to change notification settings - Fork 40
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
Fix/min size #77
base: master
Are you sure you want to change the base?
Fix/min size #77
Conversation
guessCellSize is a utility function it does not use any values of the terminal so should be treated as such
@@ -22,6 +22,11 @@ import ( | |||
|
|||
const termOverlay = fyne.ThemeColorName("termOver") | |||
|
|||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look like they should be using constants and not vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did think about that, but someone may want to change these,default or such during a build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is global variables the right way to expose this though? It's not a pattern generally embraced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should probably avoid global variables unless they are constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not force the terminal to always be that big
cellSize := guessCellSize() | ||
w.Resize(fyne.NewSize(cellSize.Width*80, cellSize.Height*24)) | ||
minSize := fyne.NewSize(cellSize.Width*DefaultCols, cellSize.Height*DefaultRows) | ||
t.SetMinSize(minSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the same behaviour as before, it is setting the preferred size to be the minimum size - so now we cannot make the terminal any smaller.
Sounds bad for usage on mobile or even desktops with small space etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the real issue is that we clear the cell data on a resize and we shouldn't we should just not render it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, well the reason they are cleared is because we only want as many cells as shown on screen - so we don't leave the "ones off the end of the line" in memory for later - we re-use them on cells further down and clear out extras.
The algorithm could be improved to dis-use the ones from the end of the row instead of re-using and just removing those not in use at the end of the view?
@@ -22,6 +22,11 @@ import ( | |||
|
|||
const termOverlay = fyne.ThemeColorName("termOver") | |||
|
|||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is global variables the right way to expose this though? It's not a pattern generally embraced.
When resizing below 80*24 rows/cols the rendering breaks with duplicate lines, lost content etc. This change prevents this by fixing the size to a minimum while allowing the previous behaviour.