first wave of addressing reviews

crates/re_data_source/src/data_loader/loader_archetype.rs

@@ -32,10 +32,10 @@ impl DataLoader for ArchetypeLoader {
             .with_context(|| format!("Failed to read file {filepath:?}"))?;
         let contents = std::borrow::Cow::Owned(contents);
 
-        self.load_from_path_contents(store_id, filepath, contents, tx)
+        self.load_from_file_contents(store_id, filepath, contents, tx)
     }
 
-    fn load_from_path_contents(
+    fn load_from_file_contents(
         &self,
         _store_id: re_log_types::StoreId,
         filepath: std::path::PathBuf,
@@ -46,36 +46,34 @@ impl DataLoader for ArchetypeLoader {
 
         let entity_path = EntityPath::from_file_path(&filepath);
 
-        let timepoint = TimePoint::timeless();
+        let mut timepoint = TimePoint::timeless();
         // TODO(cmc): log these once heuristics (I think?) are fixed
-        // if let Ok(metadata) = filepath.metadata() {
-        //     use re_log_types::{Time, Timeline};
-        //
-        //     if let Some(created) = metadata.created().ok().and_then(|t| Time::try_from(t).ok()) {
-        //         timepoint.insert(Timeline::new_temporal("created_at"), created.into());
-        //     }
-        //     if let Some(modified) = metadata
-        //         .modified()
-        //         .ok()
-        //         .and_then(|t| Time::try_from(t).ok())
-        //     {
-        //         timepoint.insert(Timeline::new_temporal("modified_at"), modified.into());
-        //     }
-        //     if let Some(accessed) = metadata
-        //         .accessed()
-        //         .ok()
-        //         .and_then(|t| Time::try_from(t).ok())
-        //     {
-        //         timepoint.insert(Timeline::new_temporal("accessed_at"), accessed.into());
-        //     }
-        // }
-
-        let extension = filepath
-            .extension()
-            .unwrap_or_default()
-            .to_ascii_lowercase()
-            .to_string_lossy()
-            .to_string();
+        if false {
+            if let Ok(metadata) = filepath.metadata() {
+                use re_log_types::{Time, Timeline};
+
+                if let Some(created) = metadata.created().ok().and_then(|t| Time::try_from(t).ok())
+                {
+                    timepoint.insert(Timeline::new_temporal("created_at"), created.into());
+                }
+                if let Some(modified) = metadata
+                    .modified()
+                    .ok()
+                    .and_then(|t| Time::try_from(t).ok())
+                {
+                    timepoint.insert(Timeline::new_temporal("modified_at"), modified.into());
+                }
+                if let Some(accessed) = metadata
+                    .accessed()
+                    .ok()
+                    .and_then(|t| Time::try_from(t).ok())
+                {
+                    timepoint.insert(Timeline::new_temporal("accessed_at"), accessed.into());
+                }
+            }
+        }
+
+        let extension = crate::extension(&filepath);
 
         let mut rows = Vec::new();
 

crates/re_data_source/src/data_loader/loader_rrd.rs

@@ -23,13 +23,7 @@ impl crate::DataLoader for RrdLoader {
 
         re_tracing::profile_function!(filepath.display().to_string());
 
-        let extension = filepath
-            .extension()
-            .unwrap_or_default()
-            .to_ascii_lowercase()
-            .to_string_lossy()
-            .to_string();
-
+        let extension = crate::extension(&filepath);
         if extension != "rrd" {
             return Ok(()); // simply not interested
         }
@@ -51,7 +45,7 @@ impl crate::DataLoader for RrdLoader {
         Ok(())
     }
 
-    fn load_from_path_contents(
+    fn load_from_file_contents(
         &self,
         // NOTE: The Store ID comes from the rrd file itself.
         _store_id: re_log_types::StoreId,
@@ -61,6 +55,11 @@ impl crate::DataLoader for RrdLoader {
     ) -> Result<(), crate::DataLoaderError> {
         re_tracing::profile_function!(filepath.display().to_string());
 
+        let extension = crate::extension(&filepath);
+        if extension != "rrd" {
+            return Ok(()); // simply not interested
+        }
+
         let version_policy = re_log_encoding::decoder::VersionPolicy::Warn;
         let contents = std::io::Cursor::new(contents);
         let decoder = match re_log_encoding::decoder::Decoder::new(version_policy, contents) {

crates/re_data_source/src/data_loader/mod.rs

@@ -87,17 +87,19 @@ pub trait DataLoader: Send + Sync {
     /// filesystem available to begin with.
     /// `path` is guaranteed to be a file path.
     ///
+    /// When running on the web (wasm), `filepath` only contains the file name.
+    ///
     /// ## Error handling
     ///
-    /// Most implementers of `load_from_path_contents` are expected to be asynchronous in nature.
+    /// Most implementers of `load_from_file_contents` are expected to be asynchronous in nature.
     ///
     /// Asynchronous implementers should make sure to fail early (and thus synchronously) when
     /// possible (e.g. didn't even manage to open the file).
     /// Otherwise, they should log errors that happen in an asynchronous context.
     ///
     /// If a [`DataLoader`] has no interest in the given file, it should successfully return
     /// without pushing any data into `tx`.
-    fn load_from_path_contents(
+    fn load_from_file_contents(
         &self,
         store_id: re_log_types::StoreId,
         filepath: std::path::PathBuf,
@@ -135,23 +137,48 @@ pub enum LoadedData {
 }
 
 impl From<DataRow> for LoadedData {
+    #[inline]
     fn from(value: DataRow) -> Self {
         Self::DataRow(value)
     }
 }
 
 impl From<ArrowMsg> for LoadedData {
+    #[inline]
     fn from(value: ArrowMsg) -> Self {
         LoadedData::ArrowMsg(value)
     }
 }
 
 impl From<LogMsg> for LoadedData {
+    #[inline]
     fn from(value: LogMsg) -> Self {
         LoadedData::LogMsg(value)
     }
 }
 
+impl LoadedData {
+    /// Pack the data into a [`LogMsg`].
+    pub fn into_log_msg(
+        self,
+        store_id: &re_log_types::StoreId,
+    ) -> Result<LogMsg, re_log_types::DataTableError> {
+        match self {
+            Self::DataRow(row) => {
+                let mut table =
+                    re_log_types::DataTable::from_rows(re_log_types::TableId::new(), [row]);
+                table.compute_all_size_bytes();
+
+                Ok(LogMsg::ArrowMsg(store_id.clone(), table.to_arrow_msg()?))
+            }
+
+            Self::ArrowMsg(msg) => Ok(LogMsg::ArrowMsg(store_id.clone(), msg)),
+
+            Self::LogMsg(msg) => Ok(msg),
+        }
+    }
+}
+
 // ---
 
 /// Keeps track of all builtin [`DataLoader`]s.

crates/re_data_source/src/data_source.rs

@@ -64,7 +64,7 @@ impl DataSource {
                 true // No dots. Weird. Let's assume it is a file path.
             } else if parts.len() == 2 {
                 // Extension or `.com` etc?
-                crate::is_known_file_extension(parts[1])
+                crate::is_supported_file_extension(parts[1])
             } else {
                 false // Too many dots; assume an url
             }
@@ -157,7 +157,7 @@ impl DataSource {
                 // decide to use it depending on whether they want to share a common recording
                 // or not.
                 let store_id = re_log_types::StoreId::random(re_log_types::StoreKind::Recording);
-                crate::load_from_path_contents(
+                crate::load_from_file_contents(
                     &store_id,
                     file_source,
                     &std::path::PathBuf::from(file_contents.name),

crates/re_data_source/src/lib.rs

@@ -22,7 +22,7 @@ pub use self::data_loader::{
     iter_loaders, ArchetypeLoader, DataLoader, DataLoaderError, LoadedData, RrdLoader,
 };
 pub use self::data_source::DataSource;
-pub use self::load_file::load_from_path_contents;
+pub use self::load_file::{extension, load_from_file_contents};
 pub use self::web_sockets::connect_to_ws_url;
 
 #[cfg(not(target_arch = "wasm32"))]
@@ -49,7 +49,17 @@ pub const SUPPORTED_MESH_EXTENSIONS: &[&str] = &["glb", "gltf", "obj"];
 
 pub const SUPPORTED_RERUN_EXTENSIONS: &[&str] = &["rrd"];
 
-pub(crate) fn is_known_file_extension(extension: &str) -> bool {
+/// All file extension supported by our builtin [`DataLoader`]s.
+pub fn supported_extensions() -> impl Iterator<Item = &'static str> {
+    SUPPORTED_RERUN_EXTENSIONS
+        .iter()
+        .chain(SUPPORTED_IMAGE_EXTENSIONS)
+        .chain(SUPPORTED_MESH_EXTENSIONS)
+        .copied()
+}
+
+/// Is this a supported file extension by any of our builtin [`DataLoader`]s?
+pub fn is_supported_file_extension(extension: &str) -> bool {
     SUPPORTED_IMAGE_EXTENSIONS.contains(&extension)
         || SUPPORTED_MESH_EXTENSIONS.contains(&extension)
         || SUPPORTED_RERUN_EXTENSIONS.contains(&extension)

crates/re_data_source/src/load_file.rs

@@ -48,7 +48,7 @@ pub fn load_from_path(
 /// and handled directly by the [`crate::DataLoader`]s themselves (i.e. they're logged).
 ///
 /// `path` is only used for informational purposes, no data is ever read from the filesystem.
-pub fn load_from_path_contents(
+pub fn load_from_file_contents(
     store_id: &re_log_types::StoreId,
     file_source: FileSource,
     filepath: &std::path::Path,
@@ -77,7 +77,7 @@ pub fn load_from_path_contents(
 
 /// Empty string if no extension.
 #[inline]
-fn extension(path: &std::path::Path) -> String {
+pub fn extension(path: &std::path::Path) -> String {
     path.extension()
         .unwrap_or_default()
         .to_ascii_lowercase()
@@ -89,8 +89,8 @@ fn extension(path: &std::path::Path) -> String {
 ///
 /// This does _not_ access the filesystem.
 #[inline]
-fn is_builtin(path: &std::path::Path, is_dir: bool) -> bool {
-    !is_dir && crate::is_known_file_extension(&extension(path))
+pub fn is_associated_with_builtin_loader(path: &std::path::Path, is_dir: bool) -> bool {
+    !is_dir && crate::is_supported_file_extension(&extension(path))
 }
 
 /// Prepares an adequate [`re_log_types::StoreInfo`] [`LogMsg`] given the input.
@@ -107,7 +107,7 @@ fn prepare_store_info(
     let app_id = re_log_types::ApplicationId(path.display().to_string());
     let store_source = re_log_types::StoreSource::File { file_source };
 
-    let is_builtin = is_builtin(path, is_dir);
+    let is_builtin = is_associated_with_builtin_loader(path, is_dir);
     let is_rrd = crate::SUPPORTED_RERUN_EXTENSIONS.contains(&extension(path).as_str());
 
     (!is_rrd && is_builtin).then(|| {
@@ -138,7 +138,7 @@ fn load(
     contents: Option<std::borrow::Cow<'_, [u8]>>,
 ) -> Result<std::sync::mpsc::Receiver<LoadedData>, DataLoaderError> {
     let extension = extension(path);
-    let is_builtin = is_builtin(path, is_dir);
+    let is_builtin = is_associated_with_builtin_loader(path, is_dir);
 
     if !is_builtin {
         return if extension.is_empty() {
@@ -169,18 +169,18 @@ fn load(
                     if let Some(contents) = contents.as_deref() {
                         let contents = Cow::Borrowed(contents.as_ref());
 
-                        if let Err(err) = loader.load_from_path_contents(
+                        if let Err(err) = loader.load_from_file_contents(
                             store_id,
                             path.clone(),
                             contents,
                             tx_loader,
                         ) {
-                            re_log::error!(?path, loader = loader.name(), %err, "failed to load data from file");
+                            re_log::error!(?path, loader = loader.name(), %err, "Failed to load data from file");
                         }
                     } else if let Err(err) =
                         loader.load_from_path(store_id, path.clone(), tx_loader)
                     {
-                        re_log::error!(?path, loader = loader.name(), %err, "failed to load data from file");
+                        re_log::error!(?path, loader = loader.name(), %err, "Failed to load data from file");
                     }
                 }
             });
@@ -191,9 +191,9 @@ fn load(
                     let contents = Cow::Borrowed(contents);
 
                     if let Err(err) =
-                        loader.load_from_path_contents(store_id, path.clone(), contents, tx_loader)
+                        loader.load_from_file_contents(store_id, path.clone(), contents, tx_loader)
                     {
-                        re_log::error!(?path, loader = loader.name(), %err, "failed to load data from file");
+                        re_log::error!(?path, loader = loader.name(), %err, "Failed to load data from file");
                     }
                 }
             });
@@ -221,35 +221,18 @@ fn send(
         move || {
             // ## Ignoring channel errors
             //
-            // Not our problem whether or not the other end has hanged up, but we still want to
+            // Not our problem whether or not the other end has hung up, but we still want to
             // poll the channel in any case so as to make sure that the data producer
             // doesn't get stuck.
             for data in rx_loader {
-                match data {
-                    LoadedData::DataRow(row) => {
-                        let mut table =
-                            re_log_types::DataTable::from_rows(re_log_types::TableId::new(), [row]);
-                        table.compute_all_size_bytes();
-
-                        let arrow_msg = match table.to_arrow_msg() {
-                            Ok(arrow_msg) => arrow_msg,
-                            Err(err) => {
-                                re_log::error!(%err, %store_id, "couldn't serialize component data");
-                                continue;
-                            }
-                        };
-
-                        tx.send(LogMsg::ArrowMsg(store_id.clone(), arrow_msg)).ok();
+                let msg = match data.into_log_msg(&store_id) {
+                    Ok(msg) => msg,
+                    Err(err) => {
+                        re_log::error!(%err, %store_id, "Couldn't serialize component data");
+                        continue;
                     }
-
-                    LoadedData::ArrowMsg(msg) => {
-                        tx.send(LogMsg::ArrowMsg(store_id.clone(), msg)).ok();
-                    }
-
-                    LoadedData::LogMsg(msg) => {
-                        tx.send(msg).ok();
-                    }
-                }
+                };
+                tx.send(msg).ok();
             }
 
             tx.quit(None).ok();

crates/re_viewer/src/app.rs

@@ -1297,12 +1297,7 @@ fn file_saver_progress_ui(egui_ctx: &egui::Context, background_tasks: &mut Backg
 #[cfg(not(target_arch = "wasm32"))]
 fn open_file_dialog_native() -> Vec<std::path::PathBuf> {
     re_tracing::profile_function!();
-    let supported: Vec<_> = re_data_source::SUPPORTED_RERUN_EXTENSIONS
-        .iter()
-        .chain(re_data_source::SUPPORTED_IMAGE_EXTENSIONS)
-        .chain(re_data_source::SUPPORTED_MESH_EXTENSIONS)
-        .copied()
-        .collect();
+    let supported: Vec<_> = re_data_source::supported_extensions().collect();
     rfd::FileDialog::new()
         .add_filter("Supported files", &supported)
         .pick_files()
@@ -1311,12 +1306,8 @@ fn open_file_dialog_native() -> Vec<std::path::PathBuf> {
 
 #[cfg(target_arch = "wasm32")]
 async fn async_open_rrd_dialog() -> Vec<re_data_source::FileContents> {
-    let supported: Vec<_> = re_data_source::SUPPORTED_RERUN_EXTENSIONS
-        .iter()
-        .chain(re_data_source::SUPPORTED_IMAGE_EXTENSIONS)
-        .chain(re_data_source::SUPPORTED_MESH_EXTENSIONS)
-        .copied()
-        .collect();
+    let supported: Vec<_> = re_data_source::supported_extensions().collect();
+
     let files = rfd::AsyncFileDialog::new()
         .add_filter("Supported files", &supported)
         .pick_files()