Ultimaker / Cura

3D printer / slicing GUI built on top of the Uranium framework
GNU Lesser General Public License v3.0
6.22k stars 2.08k forks source link

Removable Media Whitelist #10915

Closed zxc8027 closed 3 years ago

zxc8027 commented 3 years ago

Is your feature request related to a problem?

At The Construct @ RIT, we use Cura on shared, public accounts on desktops. As a result, it is very common for users to use USB drives to transfer STL files. In fact, we provide a supply of USB drives for transferring files from personal laptops to the desktops. With the provided saving to removable media option, they can result in the file being saved to one of those drives, which a lot of people don’t remove during or after using Cura, instead of the SD cards (we label as PRINTER_SD) or USB drives (we label as PRINTER_USB) for the printer, which can result in user confusion for where the files went, then proceed to re-slice the file and get billed for it.

Describe the solution you'd like

For our use case, it would be helpful to have an optional, configurable whitelist of case-insensitive drive names to filter for the removable media saving option. Making the whitelist a blacklist may also be a useful option, but we don’t have this use case. Allowing regular expressions, or other rules like “starts with”, “ends with”, or “contains” may also be useful, but we don’t have this use case either since we just use PRINTER_SD and PRINTER_USB.

Describe alternatives you've considered

To get around this, we wrapped the WindowsRemovableDrivePlugin class to apply a filter to the results of checkRemovableDrives. There is also an external configuration for the whitelist since we didn’t want to hard-code the names to whitelist for. The specific code changes we do currently aren’t public but can be made public on request if it helps our use case, and potentially other makerspaces looking to use Cura. We can also help with the related code changes if some guidance one what to change can be given.

Affected users and/or printers

This feature request affects a combination of the users (students, faculty, and alumni) to reduce user confusion and errors, as well as to Lab Managers for maintaining the systems and helping users.

Additional information & file uploads

No response

fvrmr commented 3 years ago

Hi @zxc8027 thank you for your feature request. I have discussed this with a developer and this has no priority for us to implement. A pull request is more than welcome, since you've already a workaround.

I will close this issue, I hope you understand.

zxc8027 commented 2 years ago

I am starting to look into this as something that would be made as a pull request. Thinking about potential requirements and features, I see this type of feature being able to be divided into two parts:

While we could continue using a fork of the plugin, I would prefer us to use the normal Cura releases and potentially have this functionality for others. However, I don't find UI fun to work with and not be able to provide translations. So, I wanted to ask if there would be interest in me creating a PR with just the internal filter code and any design decisions for the filter, and leave the UI to configure it up to someone else.

If the answer is no, then my thoughts below will be implemented for The Construct @ RIT. If the answer is yes, I would like some feedback on my ideas for a solution, which includes some use cases we don't have.

Use Cases

After some thinking, here are some use cases I was able to think of:

Filter Schema Design

For the state of the filter, I was thinking of a schema similar to the following if it were to be JSON:

[
  {
    "string": "test1",
    "type": "blacklist",
    "method": "exact"
  },
  {
    "string": "test2",
    "type": "blacklist",
    "method": "contains"
  },
  {
    "string": "test3",
    "type": "whitelist",
    "method": "exact"
  }
]

The schema above is a list that contains 3 parts:

If the file is missing or empty, it would be assumed that nothing should be filtered.

Questions

For a proper implementation, beyond feedback for what is posted above, I have the following questions:

Ghostkeeper commented 2 years ago

So, I wanted to ask if there would be interest in me creating a PR with just the internal filter code and any design decisions for the filter, and leave the UI to configure it up to someone else.

I'd gladly merge something like this. Before you do, though, consider this issue which is a duplicate: https://github.com/Ultimaker/Cura/issues/5207 It might be better to continue conversation there or at least take its requests into account.

For a pull request for this, we'll require:

For the requirement to keep it maintainable, I'd advise you to try KISS, i.e. keep the scope as small as possible to start with, then expand if necessary. For instance, you could drop the method and simply always use regex.

For plugins that store a state, is there is an intended method to do this?

The preferences file is preferable.

The filter could probably be unit tested. Are there any expectations of unit tests for plugins, and are there any good example plugins that have unit tests already?

The USB printing plug-in doesn't have a test suite yet. Probably because testing that without actual hardware attached to the CI computer is not simple and mostly useless, but of course you're welcome to try. We don't require complete code coverage but anything that can run on the Github Runners (=2-core Ubuntu VMs) within a reasonable amount of time (within a second or so) should be good. We search for tests in the tests subfolder using pytest. Here is an example of a test suite in a plug-in: https://github.com/Ultimaker/Uranium/blob/master/plugins/Tools/ScaleTool/tests/TestScaleTool.py

zxc8027 commented 2 years ago

With the next school semester starting up for me, I anticipate I will be able to get something done for this before the end of the month. Thanks for answering my initial questions, but I have a few follow-ups.

I'd gladly merge something like this. Before you do, though, consider this issue which is a duplicate: #5207 It might be better to continue conversation there or at least take its requests into account.

I read through the issue and I can't quite make out how it is the same or similar issue. That appears to be for USB printing over a COM port with no mentions to the removable media plugin we want to add a whitelist to.

If there is a UI component I think we should make that quite prominent, e.g. in the printer set-up actions, but for most users the current workflow works acceptably, so we don't want to break their workflow. that it doesn't break anything for ex

Just to make sure, is what I mentioned before about us working on the functional component and someone else who wants this to work on the graphical component fine? I can do the graphical component for others if it allows us to move away from patching Cura, but I don't find UIs that fun.

that it's maintainable for the future. This means that the added complexity is limited, especially outside of the USB printing plug-in.

Something I thought I brought up before is 2 ways I thought about implementing it, but I didn't. My thoughts are I can implement a filter like this using either a single, iterative method or I use the decorator pattern. Doing it functionally with a lot of loops would potentially make additions more complex and harder to understand, while the decorator pattern could be simple to add new features (new filters) do since it just basically just building a filter, but it is more complex in terms of method calls and memory usage. While trying out an implementation and writing tests, I determined the decorator pattern would prevent a whitelist from being formed by 2 disjoint terms (such as warning PRINTER_USB and PRINTER_SD, which is what we need). A response for this is no longer needed and discussions on this can happen in the PR.