Open
Conversation
…ilure The clipboard initialization on TableView::default was calling `unwrap` on the value returned by `Clipboard::new`. On enviroments without a window manager session (e.g. A server accessed via ssh), this would cause the application to panic. Instead, make the `clipboard` field on TableView and Optional, and simply ignore yank actions if no clipboard was available.
The arboard dependency pulls a lot of crates to give support for multiple desktop enviroments. This patch puts it behind a "clipboard" feature, which is enabled by default. Wayland support is also behind the "wayland" feature, also enabled by default. The idea is to give more flexibility to the people compiling this app for their personal use. For example: If I'm building JDbrowser for my home server, which I access via ssh, I don't need clipboard support, since I won't be able to use it. And even if I build it for my laptop, I won't need wayland support if I'm on an X11 server. Since the features are enabled by default, nothing breaks. But if someone wants to opt-out of a bunch of crates they won't use, let them!
The rusqlite crate defines an optional feature "bundled", that uses a bundled sqlite library. Adding an, also optional, feature "bundled-sqlite" to JDbrowser allows the user to benefit from that feature, wich only a single extra line on Cargo.toml
Owner
|
Thank you very much. I will go over this when I get a chance!! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello!
First of all, I love this app. It's great, and I'll definitely add it to my toolbox.
I've fixed the following issue: #10
On environments without a window manager (the issue talks about a remote server accessed via ssh), the
unwraponthe
Clipboard::newresult caused the application to panic.I've fixed this by making the
clipboardvariable onTableViewan Optional.Also, I've added a
DISABLE_CLIPBOARDenvironment variable (also suggested on the issue)The other two commits are some suggested features, based on my personal opinion.
I like to put features that depend on external crates behind a feature flag, to make my applications
more flexible for users that don't need those features.
For example, I run an X11 server, so i don't need the
arboard/wayland-data-controlfeature.In the aforementioned case - a remote server which doesn't have any kind of window managers - the whole arboard
dependency is pretty much useless.
I think it would be nice to make them an opt-out feature.
This way, by default all that nice functionality will be packed on the executable. But If someone wants to optimize the size
of the executable, they can disable the features they don't need.
Proof:
It's not much though, but it's 20M smaller and compiles faster.
The features I added regarding clipboard support are.
Both are enabled by default.
Another feature I added was "bundled-sqlite". The rusqlite crate has a "bundled" feature that allows you to use a bundled version of sqlite, instead of the one installed on the machine.
I added this because while testing JDbrowser on my personal server, the linker failed. Using a bundled version worked for me. This functionality only requires adding a line on the Cargo.toml.
As I said, these features are based on my personal preference. And I don't wanna be intrusive on your code. Also, making the clipboard opt-out requires some dirty #[cfg]'s
If you don't like the idea, I can remove the last two commits and leave just the bug fix.
NOTE: Maybe it would be nice to add some information about those features on the readme, but I didn't wanna mess up
anymore with the repo. If you want, I'll gladly document it.
Great work, love the app!