Skip to content

Send OpenFile request with kind field #890

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −16
Open

Send OpenFile request with kind field #890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions crates/amalthea/src/comm/data_explorer_comm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ pub enum SearchSchemaSortOrder {
}

/// Possible values for ColumnDisplayType
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum ColumnDisplayType {
#[serde(rename = "number")]
#[strum(to_string = "number")]
Expand Down Expand Up @@ -754,7 +754,7 @@ pub enum ColumnDisplayType {
}

/// Possible values for Condition in RowFilter
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum RowFilterCondition {
#[serde(rename = "and")]
#[strum(to_string = "and")]
Expand All @@ -766,7 +766,7 @@ pub enum RowFilterCondition {
}

/// Possible values for RowFilterType
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum RowFilterType {
#[serde(rename = "between")]
#[strum(to_string = "between")]
Expand Down Expand Up @@ -814,7 +814,7 @@ pub enum RowFilterType {
}

/// Possible values for Op in FilterComparison
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum FilterComparisonOp {
#[serde(rename = "=")]
#[strum(to_string = "=")]
Expand Down Expand Up @@ -842,7 +842,7 @@ pub enum FilterComparisonOp {
}

/// Possible values for TextSearchType
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum TextSearchType {
#[serde(rename = "contains")]
#[strum(to_string = "contains")]
Expand All @@ -866,7 +866,7 @@ pub enum TextSearchType {
}

/// Possible values for ColumnFilterType
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum ColumnFilterType {
#[serde(rename = "text_search")]
#[strum(to_string = "text_search")]
Expand All @@ -878,7 +878,7 @@ pub enum ColumnFilterType {
}

/// Possible values for ColumnProfileType
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum ColumnProfileType {
#[serde(rename = "null_count")]
#[strum(to_string = "null_count")]
Expand Down Expand Up @@ -906,7 +906,7 @@ pub enum ColumnProfileType {
}

/// Possible values for Method in ColumnHistogramParams
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum ColumnHistogramParamsMethod {
#[serde(rename = "sturges")]
#[strum(to_string = "sturges")]
Expand All @@ -926,7 +926,7 @@ pub enum ColumnHistogramParamsMethod {
}

/// Possible values for Kind in TableSelection
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum TableSelectionKind {
#[serde(rename = "single_cell")]
#[strum(to_string = "single_cell")]
Expand Down Expand Up @@ -954,7 +954,7 @@ pub enum TableSelectionKind {
}

/// Possible values for ExportFormat
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum ExportFormat {
#[serde(rename = "csv")]
#[strum(to_string = "csv")]
Expand All @@ -970,7 +970,7 @@ pub enum ExportFormat {
}

/// Possible values for SupportStatus
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum SupportStatus {
#[serde(rename = "unsupported")]
#[strum(to_string = "unsupported")]
Expand Down
2 changes: 1 addition & 1 deletion crates/amalthea/src/comm/help_comm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use serde::Deserialize;
use serde::Serialize;

/// Possible values for Kind in ShowHelp
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum ShowHelpKind {
#[serde(rename = "html")]
#[strum(to_string = "html")]
Expand Down
4 changes: 2 additions & 2 deletions crates/amalthea/src/comm/plot_comm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub struct PlotRenderSettings {
}

/// Possible values for PlotUnit
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum PlotUnit {
#[serde(rename = "pixels")]
#[strum(to_string = "pixels")]
Expand All @@ -76,7 +76,7 @@ pub enum PlotUnit {
}

/// Possible values for PlotRenderFormat
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum PlotRenderFormat {
#[serde(rename = "png")]
#[strum(to_string = "png")]
Expand Down
16 changes: 16 additions & 0 deletions crates/amalthea/src/comm/ui_comm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,18 @@ pub struct Range {
pub end: Position
}

/// Possible values for Kind in OpenEditor
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum OpenEditorKind {
#[serde(rename = "path")]
#[strum(to_string = "path")]
Path,

#[serde(rename = "uri")]
#[strum(to_string = "uri")]
Uri
}

/// Parameters for the DidChangePlotsRenderSettings method.
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
pub struct DidChangePlotsRenderSettingsParams {
Expand Down Expand Up @@ -133,6 +145,10 @@ pub struct OpenEditorParams {

/// The column number to jump to
pub column: i64,

/// How to interpret the 'file' argument: as a file path or as a URI. If
/// omitted, defaults to 'path'.
pub kind: OpenEditorKind,
Comment on lines +149 to +151
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the argument names are a little mixed up, I feel like if we are generalizing, it should become

// The path of the file or URI to open
pub path: String

// How to interpret the `path` argument: as a file or a URI.
// If omitted, defaults to `File`
pub kind: OpenEditorPathKind

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, strum_macros::Display, strum_macros::EnumString)]
pub enum OpenEditorPathKind {
	#[serde(rename = "file")]
	#[strum(to_string = "file")]
	File,

	#[serde(rename = "uri")]
	#[strum(to_string = "uri")]
	Uri
}

i.e. to me the ambiguous concept is a path, which is specialized to either a file or a uri.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The positron side of this PR seems to agree with me, where you have

				if (ed.kind === 'uri') {
					file = URI.parse(ed.file);
				} else {
					file = URI.file(ed.file);
				}

i.e. you have an arbitrary path, if URI you parse() it, otherwise if File you file() it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think I'd prefer not to change path as this would likely require updating the Python side. The current setup is backward compatible.

}

/// Parameters for the NewDocument method.
Expand Down
7 changes: 5 additions & 2 deletions crates/ark/src/modules/positron/frontend-methods.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@
column = 0L
) {
# Don't normalize if that's an `ark:` URI
if (!grepl("^ark:", file)) {
if (grepl("^ark:", file)) {
kind <- "uri"
} else {
kind <- "path"
file <- normalizePath(file)
}

Expand All @@ -51,7 +54,7 @@
column <- 0L
}

.ps.Call("ps_ui_navigate_to_file", file, line, column)
.ps.Call("ps_ui_navigate_to_file", file, line, column, kind)
}

#' @export
Expand Down
8 changes: 8 additions & 0 deletions crates/ark/src/ui/events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
//
//

use std::str::FromStr;

use amalthea::comm::ui_comm::OpenEditorKind;
use amalthea::comm::ui_comm::OpenEditorParams;
use amalthea::comm::ui_comm::OpenWithSystemParams;
use amalthea::comm::ui_comm::OpenWorkspaceParams;
Expand Down Expand Up @@ -63,11 +66,16 @@ pub unsafe extern "C-unwind" fn ps_ui_navigate_to_file(
file: SEXP,
line: SEXP,
column: SEXP,
uri: SEXP,
) -> anyhow::Result<SEXP> {
let kind: String = RObject::view(uri).try_into()?;
let kind = OpenEditorKind::from_str(&kind)?;

let params = OpenEditorParams {
file: RObject::view(file).try_into()?,
line: RObject::view(line).try_into()?,
column: RObject::view(column).try_into()?,
kind,
};

let event = UiFrontendEvent::OpenEditor(params);
Expand Down
Loading