-
Notifications
You must be signed in to change notification settings - Fork 283
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
Warn when closing window with unsaved changes #1230
Changes from all commits
c1df14a
1fef126
8a62f03
9e76e80
8389537
391e8e4
73fdaaf
26b4c45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
open Oni_Core; | ||
|
||
open Revery.UI; | ||
open Revery.UI.Components; | ||
|
||
type action('msg) = { | ||
label: string, | ||
msg: 'msg, | ||
}; | ||
|
||
module Styles = { | ||
open Style; | ||
|
||
let container = (~theme: Theme.t) => [backgroundColor(theme.background)]; | ||
|
||
let message = [ | ||
padding(20), | ||
paddingBottom(30) // I don't know why this is needed, but it is | ||
]; | ||
|
||
let actions = [flexDirection(`Row)]; | ||
|
||
let buttonOuter = (~isHovered, ~theme: Theme.t) => [ | ||
isHovered | ||
? backgroundColor(theme.menuSelectionBackground) | ||
: backgroundColor(theme.editorBackground), | ||
flexGrow(1), | ||
]; | ||
|
||
let buttonInner = [padding(10)]; | ||
|
||
let buttonText = (~isHovered, ~theme: Theme.t, ~font: UiFont.t) => [ | ||
fontFamily(font.fontFile), | ||
color(theme.foreground), | ||
isHovered | ||
? backgroundColor(theme.menuSelectionBackground) | ||
: backgroundColor(theme.editorBackground), | ||
fontSize(14), | ||
alignSelf(`Center), | ||
]; | ||
}; | ||
|
||
let%component button = (~text, ~onClick, ~theme, ~font, ()) => { | ||
let%hook (isHovered, setHovered) = Hooks.state(false); | ||
|
||
<Clickable onClick style={Styles.buttonOuter(~theme, ~isHovered)}> | ||
<View | ||
onMouseOver={_ => setHovered(_ => true)} | ||
onMouseOut={_ => setHovered(_ => false)} | ||
style=Styles.buttonInner> | ||
<Text style={Styles.buttonText(~isHovered, ~theme, ~font)} text /> | ||
</View> | ||
</Clickable>; | ||
}; | ||
|
||
let make = (~children as message, ~theme, ~font, ~actions, ~onAction, ()) => | ||
<View style={Styles.container(~theme)}> | ||
<View style=Styles.message> message </View> | ||
<View style=Styles.actions> | ||
{actions | ||
|> List.map(action => | ||
<button | ||
text={action.label} | ||
onClick={() => onAction(action.msg)} | ||
theme | ||
font | ||
/> | ||
) | ||
|> React.listToElement} | ||
</View> | ||
</View>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,6 +126,10 @@ type t = | |
| Sneak(Sneak.action) | ||
| PaneTabClicked(Pane.paneType) | ||
| VimDirectoryChanged(string) | ||
| WindowCloseBlocked | ||
| WindowCloseDiscardConfirmed | ||
| WindowCloseSaveAllConfirmed | ||
Comment on lines
+130
to
+131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are a bit awkward as they're essentially user intents. I also don't think it's necessarily a problem to have "action-like" messages, and with feature projects the encapsulation and lack of granularity might sometimes make event-like messages difficult. But enforcing event-like names is still very useful to stop thinking of them as functions. This is also a reason why I'm leaning more towards using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good! It seems like we're converging towards a very Elm-like architecture (ie, |
||
| WindowCloseCanceled | ||
// "Internal" effect action, see TitleStoreConnector | ||
| SetTitle(string) | ||
| Noop | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,24 +44,17 @@ let isModifiedByPath = (buffers: t, filePath: string) => { | |
); | ||
}; | ||
|
||
let ofBufferOpt = (f, buffer) => { | ||
switch (buffer) { | ||
| None => None | ||
| Some(b) => Some(f(b)) | ||
}; | ||
}; | ||
|
||
let applyBufferUpdate = bufferUpdate => | ||
ofBufferOpt(buffer => Buffer.update(buffer, bufferUpdate)); | ||
Option.map(buffer => Buffer.update(buffer, bufferUpdate)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, nice, thanks for cleaning this up! |
||
|
||
let setIndentation = indent => | ||
ofBufferOpt(buffer => Buffer.setIndentation(indent, buffer)); | ||
Option.map(buffer => Buffer.setIndentation(indent, buffer)); | ||
|
||
let disableSyntaxHighlighting = | ||
ofBufferOpt(buffer => Buffer.disableSyntaxHighlighting(buffer)); | ||
Option.map(buffer => Buffer.disableSyntaxHighlighting(buffer)); | ||
|
||
let setModified = modified => | ||
ofBufferOpt(buffer => Buffer.setModified(modified, buffer)); | ||
Option.map(buffer => Buffer.setModified(modified, buffer)); | ||
|
||
let reduce = (state: t, action: Actions.t) => { | ||
switch (action) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
type t = | ||
| UnsavedBuffersWarning; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,7 @@ type t = { | |
pane: Pane.t, | ||
searchPane: Feature_Search.model, | ||
focus: Focus.stack, | ||
modal: option(Modal.t), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to combine the modal and context menu model, so that a modal and context menu can't both be visible at the same time. Have to merge the context menu PR before doing that though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool. I think the context menu should be unblocked now (#1232 brought in the |
||
}; | ||
|
||
let create: unit => t = | ||
|
@@ -102,4 +103,5 @@ let create: unit => t = | |
pane: Pane.initial, | ||
searchPane: Feature_Search.initial, | ||
focus: Focus.initial, | ||
modal: None, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
open Revery; | ||
open Revery.UI; | ||
|
||
open Oni_Core; | ||
open Utility; | ||
open Oni_Model; | ||
open Oni_Components; | ||
open Actions; | ||
|
||
module Styles = { | ||
open Style; | ||
|
||
let overlay = [ | ||
backgroundColor(Color.hex("#0004")), | ||
position(`Absolute), | ||
top(0), | ||
left(0), | ||
right(0), | ||
bottom(0), | ||
alignItems(`Center), | ||
justifyContent(`Center), | ||
overflow(`Hidden), | ||
flexDirection(`Column), | ||
pointerEvents(`Allow), | ||
]; | ||
|
||
let text = (~theme: Theme.t, ~font: UiFont.t) => [ | ||
fontFamily(font.fontFile), | ||
color(theme.editorForeground), | ||
backgroundColor(theme.editorBackground), | ||
fontSize(14), | ||
]; | ||
|
||
let files = [padding(10)]; | ||
|
||
let file = (~theme: Theme.t, ~font: UiFont.t) => [ | ||
fontFamily(font.fontFile), | ||
color(theme.foreground), | ||
backgroundColor(theme.editorBackground), | ||
fontSize(14), | ||
]; | ||
}; | ||
|
||
let unsavedBufferWarning = | ||
(~workingDirectory, ~buffers, ~theme, ~uiFont as font, ()) => { | ||
let modifiedFiles = | ||
buffers | ||
|> IntMap.to_seq | ||
|> Seq.map(snd) | ||
|> Seq.filter(Buffer.isModified) | ||
|> Seq.map(Buffer.getFilePath) | ||
|> List.of_seq | ||
|> OptionEx.values | ||
|> List.map( | ||
Path.toRelative(~base=Option.value(workingDirectory, ~default="")), | ||
); | ||
|
||
<MessageBox | ||
actions=MessageBox.[ | ||
{label: "Discard Changes", msg: WindowCloseDiscardConfirmed}, | ||
{label: "Cancel", msg: WindowCloseCanceled}, | ||
{label: "Save All", msg: WindowCloseSaveAllConfirmed}, | ||
] | ||
onAction={GlobalContext.current().dispatch} | ||
theme | ||
font> | ||
<Text | ||
style={Styles.text(~theme, ~font)} | ||
text="You have unsaved changes in the following files:" | ||
/> | ||
<View style=Styles.files> | ||
{modifiedFiles | ||
|> List.map(text => <Text style={Styles.file(~theme, ~font)} text />) | ||
|> React.listToElement} | ||
</View> | ||
<Text | ||
style={Styles.text(~theme, ~font)} | ||
text="Would you like to to save them before closing?" | ||
/> | ||
</MessageBox>; | ||
}; | ||
|
||
let make = (~state: State.t, ()) => { | ||
let State.{theme, uiFont, buffers, workspace, _} = state; | ||
let workingDirectory = | ||
Option.map(ws => ws.Workspace.workingDirectory, workspace); | ||
|
||
switch (state.modal) { | ||
| None => React.empty | ||
| Some(modal) => | ||
<View style=Styles.overlay> | ||
{switch (modal) { | ||
| UnsavedBuffersWarning => | ||
<unsavedBufferWarning workingDirectory buffers theme uiFont /> | ||
}} | ||
</View> | ||
}; | ||
}; |
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.
There's some bugginess with the content size calculation here which causes the content to overflow unless adding some extra padding.
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.
Seems like the text wrapping is what's causing the issue here. If the text doesn't wrap then everything measures as expected.
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.
Ah, ya, it sounds like from our conversation there may be a bug with the
measure
method of theTextNode
🤔