PiSCSI / piscsi

PiSCSI allows a Raspberry Pi to function as emulated SCSI devices (hard disk, CD-ROM, and others) for vintage SCSI-based computers and devices. This is a fork of the RaSCSI project by GIMONS.
https://piscsi.org
BSD 3-Clause "New" or "Revised" License
545 stars 82 forks source link

Web UI: Rework the Attach Device section to be universal #1393

Closed rdmark closed 11 months ago

rdmark commented 11 months ago

What was known as the "Attach Peripheral Device" has been reworked as "Attach Device" and now lists all devices that the piscsi backend can support. As part of this change, Attach Device has been reimagined as the primary way one would attach a disk image as well, and moved up above the Image Management section.

uweseimet commented 11 months ago

@rdmark Yes, the SAHD type is offered now. What confuses me is that an image file is displayed for "SCHD", but not for any other drive. In the "Attach Device" section I would expect no image files to be attached by default. Screenshot_20231201_173323 Instead of "Attach Device" the German UI should say something like "Gerät hinzufügen". And instead of "Image-Dateiverwaltung" it should say "Imagedatei-Verwaltung", or better "Image-Datei-Verwaltung". Strictly speaking it should also say "Datei-Upload-Funktionalität" instead of "Dateiupload-Funtionalität".

rdmark commented 11 months ago

@uweseimet The SCHD device cannot be attached without specifying an image file, can it? That’s why I filtered out the “None” option for it. If SAHD was a known device type I’d do the same for it.

Thanks for the German localization suggestions. I’ll get them pushed to the branch later. (In fact, I'll do that in a later PR, since translating new strings require regenerating the po file which adds a lot of noise to the changeset.)

uweseimet commented 11 months ago

@rdmark Yes, you have to provide an image file for SCHD, but I'm not sure whether the UI should suggest one. From all the image files I have in my folder, it suggests most likely one I am not interested in. IMO some kind of a placeholder would be better, if technically possible. The UI might say in the color red "Select an image file", for instance. Does the UI select a different filename if the one it would have suggested (aranym.img in my case) is already attached? How does the UI decide which name to suggest?

rdmark commented 11 months ago

@uweseimet I like the idea of having the default selected item being something like "Choose an image file". This change has been pushed now, alongside a number of small improvements. For the removable drive types, I still want to allow attaching them without specifying an image file. This is a handy workflow when when you want prepare for inserting an image after booting the host system, or preparing a set of devices to be saved as a default configuration.

The logic for creating the list of images to suggest for a particular device type is:

So basically, the first matching file alphabetically gets suggested by default.

uweseimet commented 11 months ago

@rdmark Sure, SCRM is a different case. I was just referring to SCHD. I will have a look at the changes later.

uweseimet commented 11 months ago

@rdmark I noticed that for SCRM you also ask the user for choosing an image file. Maybe this should better say "Optionally choose an image file" or something similar? Otherwise it's not clear for the user that you can simply proceed without attaching a file yet.

rdmark commented 11 months ago

@uweseimet I pushed a change now that I think makes for an intuitive form UI. For non-removable disk drives, it should now default to a greyed-out image file selection, and with html5 form validation in place that forces you to choose a file. For removable disk drives, the default "None" selection should signal that you can attach without an image file, I think.

uweseimet commented 11 months ago

@rdmark I cannot confirm that, I'm afraid, see screenshot. There is nothing greyed out. It would also be nice if there was some kind of alignment for "Identify as:", "Image file:" and so on. Are there no community members that can help with completing the translations?

Screenshot_20231203_131002

rdmark commented 11 months ago

@uweseimet You seem to not be running the very latest code there. You may have to force the web UI to restart.

I agree that the alignment is a bit messy right now. Let me think about how to make it cleaner. However there is only so much you can with the nesting of web forms and tables, while also making it work on very old browsers running on vintage computers.

Regarding translations, our process right now is to ask for community members to translate 2 weeks before we plan to tag a release. Web UI strings are often in flux as we're adding and refining features.

uweseimet commented 11 months ago

@rdmark It's strange, now still nothing is greyed out, but SAHD is listed as "Unknown fixed disk drive". How do you know that it is a fixed disk drive? It could be anything. In case you derive this from "HD" at the end: The UI should not try to be too smart. This can backfire ;-). Screenshot_20231203_193145

rdmark commented 11 months ago

@uweseimet If you expand the dropdowns you should be able to see that the "Choose a file" option is greyed out. (It's up to the browser how it wants to render the disabled property.)

How do you know that it is a fixed disk drive? It could be anything. In case you derive this from "HD" at the end: The UI should not try to be too smart. This can backfire ;-).

I know by asking piscsi if the device is supports_file but not removable.

rdmark commented 11 months ago

I pushed a small css style update that adds a width style to the attach device dropdowns, so that they line up under most circumstances. Tested that it looks good on my iPhone as well.

sonarcloud[bot] commented 11 months ago

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

uweseimet commented 11 months ago

@uweseimet If you expand the dropdowns you should be able to see that the "Choose a file" option is greyed out. (It's up to the browser how it wants to render the disabled property.)

Are you sure? Neither Firefox nor Chrome, the latter having the biggest market share by far, grey it out on my machine.

How do you know that it is a fixed disk drive? It could be anything. In case you derive this from "HD" at the end: The UI should not try to be too smart. This can backfire ;-).

I know by asking piscsi if the device is supports_file but not removable.

This may work, indeed.

rdmark commented 11 months ago
Screenshot 2023-12-08 at 10 23 36

I fed some mock data via piscsi_cmds.get_device_types() to test the happy path:


        device_types["SAHD"] = {'removable': False, 'supports_file': True, 'params': {}, 'block_sizes': [4096, 2048, 1024, 512]}
        device_types["SARM"] = {'removable': True, 'supports_file': True, 'params': {}, 'block_sizes': [4096, 2048, 1024, 512]}
        device_types["SAAA"] = {'removable': False, 'supports_file': False, 'params': {'foo': '123', 'bar': 'abc'}}
rdmark commented 11 months ago
Screenshot 2023-12-08 at 10 30 58

This is what the greyed out option looks like in Firefox.