Skip to content

Conversation

shiruken1
Copy link

@shiruken1 shiruken1 commented Jul 13, 2025

Fixes #1601 and gives text preview for "text/*" and "application/sql" mimetypes

@shiruken1 shiruken1 changed the title Opens PropertiesWindow on 1-click after waiting 2-click timeout WIP for issues#1601 Jul 13, 2025
…le selection. Next: draw info instead of same dir contents
@shiruken1 shiruken1 changed the title WIP for issues#1601 WIP for issue#1601 Jul 17, 2025
@shiruken1 shiruken1 changed the title WIP for issue#1601 WIP for issue#1601 (Almost done) Jul 28, 2025
@shiruken1 shiruken1 changed the title WIP for issue#1601 (Almost done) Issue #1601 (Need help with UI) Jul 29, 2025
@shiruken1
Copy link
Author

shiruken1 commented Jul 29, 2025

Screenshot from 2025-07-29 13-44-49

@jeremypw @alainm23 wutcha'll think? I'd like to pad the Box to the middle of the colpane, but this is my first time coding in Vala (Love it!) and using the Gtk/Gdk libs, which i haven't figured out how to "pad" like in CSS. Open to feedback!

P.S: Oh yeah, and padding between the info_grid items, as well

@shiruken1 shiruken1 marked this pull request as ready for review July 29, 2025 18:48
@shiruken1
Copy link
Author

@leonardo-lemos I took it out of draft mode a little too quickly and forgot about text preview. No clue what pattern to follow to pull that off, and I know it's technically out of scope for the issue, but I feel it's in the spirit of mirroring OSX' preview panel, yes?

Thoughts?

@jeremypw
Copy link
Collaborator

@shiruken1 I applaud you for having the courage to get to grips with Files code - its not easy! The screenshot looks very promising. I'll take some time to go through the code and get back to you with specific issues. I suspect we will want to have a preference switch to turn this feature on and off. @danirabbit Are you willing to extend Files in this way? I know you have expressed doubts about file previewing before but there seems to be a significant demand for it. I have no objection if implemented well.

@shiruken1
Copy link
Author

shiruken1 commented Jul 30, 2025

@shiruken1 I applaud you for having the courage to get to grips with Files code - its not easy! The screenshot looks very promising. I'll take some time to go through the code and get back to you with specific issues. I suspect we will want to have a preference switch to turn this feature on and off. @danirabbit Are you willing to extend Files in this way? I know you have expressed doubts about file previewing before but there seems to be a significant demand for it. I have no objection if implemented well.

Thanks!

Thankfully my C# coding came in handy to get my eyes adjusted (10+ years of Node and Python) but I gotta say the codebase is neat and tidy for the most part, making it easy for me to pattern-match.

I did wanna propose pulling construct_info_panel out of PropertiesWindow and placing it in AbstractPropertiesDialog, so DetailsColumn could piggy-back off of as much as possible to keep the code DRY. Lmk!

@shiruken1 shiruken1 changed the title Issue #1601 (Need help with UI) Issue #1601 (Needs UI tweaks) Jul 30, 2025
@jeremypw jeremypw changed the title Issue #1601 (Needs UI tweaks) Add preview column to Column View Jul 30, 2025
@jeremypw
Copy link
Collaborator

@shiruken1 OK, let me know if you need more help. I am not sure what remaining issue with up/down directory you mean - note that if it also occurs in main then you do not need to fix it here.

@shiruken1
Copy link
Author

@jeremypw thanks. Yeah main doesn't have an issue navigating up/down directories with the keyboard (or should I say left/right?)

Also, just to make sure, you don't feel the need for throttling anymore? You had asked:

There probably need to be some throttling to handle very rapid selection change, i.e. the preview is only displayed if the selection doesn't change for (e.g.) 100ms.

Let me know and I can put that back in. I'll be trying to figure out why navigating in and out of directories is broken, in the meantime

@jeremypw
Copy link
Collaborator

That folder navigation issue was fixed in my branch at one point. Not sure why its re-appeared - investigating.

@jeremypw
Copy link
Collaborator

You should add the throttle on the draw_details function I guess. Not on key presses - those key_press timeout ids should have been removed but seem to have reappeared after merging my branch.

@jeremypw
Copy link
Collaborator

@shiruken1 The navigation issue is caused by Miller handling the Up/Down keys - which it does not need to do now.

@jeremypw
Copy link
Collaborator

@shiruken1 I have pushed a PR showing how I suggest you throttle the drawing as well as removing the unnecessary key handling in Miller

@shiruken1
Copy link
Author

@jeremypw aaah, I guess my super duper update-main-and-feature-branch-from-upstream overwrote something of yours. My apologies and thank you for helping get this across the finish line! 🎉 Works' been a little heavy this last week, so I really appreciate it 🏆

@teamcons
Copy link

teamcons commented Oct 17, 2025

P.S: I don't like the approach, but I'm having to use the timeout to get it to work, since on_slot_selection_changed is the only way in Miller to reliably get the latest selection

Sometimes there is a race between different signals being emitted/handled and an Idle or Timeout is the simplest way to fix it. I'll look to see whether there is a better way in this case.

Vala lock() feature?

@shiruken1 shiruken1 requested a review from wpkelso October 18, 2025 12:35
@jeremypw
Copy link
Collaborator

@teamcons lock() can be useful where the race involves changes to a single property but only works within a class I think (one class cannot lock a property of a different class). In this case using a different design eliminated the race anyway.

@jeremypw
Copy link
Collaborator

@shiruken1 Once we are happy with the functionality I'll go through and fix any remaining code-style issues. They don't all show up as lint.

@shiruken1
Copy link
Author

@jeremypw sounds great. Let me know if there's anything I can do to help out further. Cheers!

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

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

Some nit-picking comments/suggestions on code-style.

  • Use var when the type is implicit in the assignment
  • Whitespace after closing brace of code blocks
  • Whitespace after statements only where the focus changes (generally speaking but this is open to interpretation and personal preference)
  • Don't make format changes to old code that is not being modified in order to minimise the diff.

@shiruken1 shiruken1 requested a review from jeremypw October 19, 2025 14:01
@jeremypw
Copy link
Collaborator

@shiruken1 Yes, sorry I should have made another PR with that many suggestions I guess. You can batch them up rather than commit separately I think. Looks like I introduced some lint which is a disadvantage of making changes on Github. Just some trailing white space - can you fix that please?

@shiruken1
Copy link
Author

No worires! I tried putting everything in commit 9068a04, but GH was still showing your comments as unresolved. Just pushed a cleanup commit to handle trailing white space 🚀

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.

(Mixed mode) clicking on a file should open its details in the next column

5 participants