Skip to content

Sort by metadata#210

Open
hacknus wants to merge 26 commits intojannistpl:mainfrom
hacknus:sort_by_metadata
Open

Sort by metadata#210
hacknus wants to merge 26 commits intojannistpl:mainfrom
hacknus:sort_by_metadata

Conversation

@hacknus
Copy link
Contributor

@hacknus hacknus commented Nov 30, 2024

Feature request to display metadata and sort by size, date etc (#165)

This PR relies on #184.

Bildschirmfoto 2024-11-30 um 13 40 24

EDIT: moved this PR over here so you can review/edit, the diffs here should be more clear once #184 is merged.

@jannistpl
Copy link
Owner

Thank you for the implementation! I'll do a review once #184 is merged and the changes here are cleaned up.
However, I think we'll have quite a few merge conflicts here when #184 is squash merged.

@hacknus
Copy link
Contributor Author

hacknus commented Nov 30, 2024

Sure! I'll solve the conflicts before your review.

@hacknus
Copy link
Contributor Author

hacknus commented Dec 6, 2024

no more merge conflicts, up to date with develop. But the issue with the column width is not solved.

Copy link
Owner

@jannistpl jannistpl left a comment

Choose a reason for hiding this comment

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

I've now found the time to do a first code review :)
But I haven't been able to test it yet...
Thanks for the work!

@jannistpl
Copy link
Owner

@hacknus what is the status of the width?

Ideally there should also be the option to display the details or, like before, to only display the file name.

@hacknus
Copy link
Contributor Author

hacknus commented Jan 2, 2025

@hacknus what is the status of the width?

I tried some more things but have not found a way for a clean fix..

Ideally there should also be the option to display the details or, like before, to only display the file name.

Yes, I can implement this.

@hacknus
Copy link
Contributor Author

hacknus commented Jan 3, 2025

Done!
But I still was not able to fix the width-issue. I need some help there, as I do not understand the widgets/space acquisition well enough.

@hacknus
Copy link
Contributor Author

hacknus commented Jan 3, 2025

Somehow the scrolling does not work anymore when I drop a file. Since it only renders the files in view,

let primary_selected = self.is_primary_selected(item);

this is never true, so it will never run the scroll_to_me() function:

if primary_selected && self.scroll_to_selection {
    row.response().scroll_to_me(Some(egui::Align::Center));
    self.scroll_to_selection = false;
}

Gotta look into this.

@jannistpl
Copy link
Owner

jannistpl commented Jan 3, 2025

Is the sort_order and sort_by in the config just the initial sorting config? Can this still be changed by the user?

@hacknus
Copy link
Contributor Author

hacknus commented Jan 3, 2025

Is the sord_order and sord_by in the config just the initial sorting config? Can this still be changed by the user?

yes, by clicking on the individual headers in the window.

@hacknus
Copy link
Contributor Author

hacknus commented Jan 3, 2025

Okay, so the scroll bug is fixed. From my side I think all that is left is the error with the width of the central panel. I think this is a general issue that concerns the right-side panel. I don't have enough experience here and would require help to fix this.
If you have additional inputs / code reviews I'll implement your suggestions.

As for now, this feature is definitely usable, but because of the bug with the width not ready.

@jannistpl
Copy link
Owner

jannistpl commented Jan 4, 2025

Okay, so the scroll bug is fixed. From my side I think all that is left is the error with the width of the central panel. I think this is a general issue that concerns the right-side panel. I don't have enough experience here and would require help to fix this. If you have additional inputs / code reviews I'll implement your suggestions.

As for now, this feature is definitely usable, but because of the bug with the width not ready.

Awesome, thanks for the update. I'll check over the next few weeks to see if I can get the width fixed. But I've never done anything with the egui-extras tables myself. So I have to try it out first :)

@hacknus
Copy link
Contributor Author

hacknus commented Jan 4, 2025

Awesome, thanks for the update. I'll check over the next few weeks to see if I can get the width fixed. But I've never done anything with the egui-extras tables myself. So I have to try it out first :)

The issue is not just with the tables, I think it is related to the issue we have with the right-side-panel where the width cannot really be set appropriately and it just extends the window to the max...
Looking forward to your inputs/findings!

@bircni
Copy link
Contributor

bircni commented Jan 5, 2025

@hacknus maybe use grid instead of table?
this fixed my issues with the width i had

@hacknus
Copy link
Contributor Author

hacknus commented Jan 5, 2025

@hacknus maybe use grid instead of table? this fixed my issues with the width i had

Might be worth looking into. But I'm not sure if the selecting/highlighting can be done as nicely in the grid.

EDIT: Also not sure if it would be as easy to only render the currently visible lines with a grid.

@hacknus
Copy link
Contributor Author

hacknus commented Jan 5, 2025

@FluxxCode is there a way how we can access the width of the width of the left panel?
nevermind, that would not help..

@hacknus
Copy link
Contributor Author

hacknus commented Jan 5, 2025

The table-bug has already been reported (one year ago...): emilk/egui#3680

It's not the same as for the width in the right-side-panel.

@jannistpl
Copy link
Owner

The table-bug has already been reported (one year ago...): emilk/egui#3680

If this is a bug in egui, I'm not sure it's worth putting a lot of energy into debugging it. At least here in this project. We should rather open a PR at egui.

The rendering problem with the grid is a good point. Unfortunately, this makes it impossible for us to use the grid as a workaround.

@hacknus
Copy link
Contributor Author

hacknus commented Jan 8, 2025

The table-bug has already been reported (one year ago...): emilk/egui#3680

If this is a bug in egui, I'm not sure it's worth putting a lot of energy into debugging it. At least here in this project. We should rather open a PR at egui.

Yes, I agree. I tried looking into the egui-code but I haven't been able to figure out where the bug originates.

# Conflicts:
#	src/config/mod.rs
#	src/data/directory_content.rs
#	src/file_dialog.rs
#	src/information_panel.rs
@jannistpl jannistpl changed the base branch from develop to main February 4, 2025 21:15
@jannistpl
Copy link
Owner

@hacknus FYI renamed branch develop to main

@jannistpl jannistpl changed the base branch from main to develop February 4, 2025 21:16
@jannistpl
Copy link
Owner

I didn't know I could just rename the branch using the Github interface, lol

# Conflicts:
#	Cargo.toml
#	src/file_dialog.rs
# Conflicts:
#	src/config/labels.rs
#	src/config/mod.rs
#	src/file_dialog.rs
# Conflicts:
#	Cargo.toml
#	src/file_dialog.rs
# Conflicts:
#	Cargo.toml
#	src/data/directory_content.rs
#	src/file_dialog.rs
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

Successfully merging this pull request may close these issues.

3 participants