Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from
Dec 8, 2023

Conversation

rdmark
Copy link
Member

@rdmark rdmark commented Dec 1, 2023

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.

  • Add "image file" dropdown for each "supports_file" device type under Attach Device
  • Under the hood, merge the two attach endpoints to reduce code duplication
  • Move Attach Device further up on the page
  • Collapse the list of images by default
  • Remove the logic that checked for overflowing bytes in images (complexity for little payoff)
  • Don't abort the Web UI when working dirs don't exist, but rather hide sections and show a warning message
  • Correct the German translation for Key
  • Refactor the integration tests to use globally defined endpoint constants

@rdmark rdmark changed the base branch from main to develop December 1, 2023 02:00
@rdmark rdmark changed the title Rdmark issue 1392 else: drive_properties = Path(CFG_DIR) / f"{file_name}.{PROPERTIES_SUFFIX}" if drive_properties.is_file(): process = file_cmd.read_drive_properties(drive_properties) process = ReturnCodeMapper.add_msg(process) if not process["status"]: return response(error=True, message=process["msg"]) conf = process["conf"] kwargs["vendor"] = conf["vendor"] kwargs["product"] = conf["product"] kwargs["revision"] = conf["revision"] kwargs["block_size"] = conf["block_size"] expected_block_size = conf["block_size"] Dec 1, 2023
@rdmark rdmark changed the title else: drive_properties = Path(CFG_DIR) / f"{file_name}.{PROPERTIES_SUFFIX}" if drive_properties.is_file(): process = file_cmd.read_drive_properties(drive_properties) process = ReturnCodeMapper.add_msg(process) if not process["status"]: return response(error=True, message=process["msg"]) conf = process["conf"] kwargs["vendor"] = conf["vendor"] kwargs["product"] = conf["product"] kwargs["revision"] = conf["revision"] kwargs["block_size"] = conf["block_size"] expected_block_size = conf["block_size"] Web UI: Rework the Attach Device section to be universal Dec 1, 2023
@rdmark rdmark force-pushed the rdmark-issue-1392 branch from d1248fc to 66545e2 Compare December 1, 2023 05:21
@rdmark rdmark force-pushed the rdmark-issue-1392 branch from 51f9157 to 6f65f24 Compare December 1, 2023 07:15
@rdmark rdmark marked this pull request as ready for review December 1, 2023 07:32
@rdmark rdmark requested a review from akuker as a code owner December 1, 2023 07:32
@rdmark rdmark mentioned this pull request Dec 1, 2023
3 tasks
@rdmark rdmark requested a review from nucleogenic December 1, 2023 12:15
@uweseimet
Copy link
Contributor

@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
Copy link
Member Author

rdmark commented Dec 2, 2023

@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
Copy link
Contributor

@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?

@uweseimet uweseimet linked an issue Dec 2, 2023 that may be closed by this pull request
3 tasks
@rdmark
Copy link
Member Author

rdmark commented Dec 2, 2023

@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:

  • Inquiry piscsi for a list of known supported file endings (server_info.mapping_info.mapping) for the device types
  • Filter images present in the images dir that match these file endings, for the particular device type, e.g. SCHD
  • Sort the resulting list of files in ascending alphabetical order (non-case sensitive)

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

@uweseimet
Copy link
Contributor

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

@uweseimet
Copy link
Contributor

@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
Copy link
Member Author

rdmark commented Dec 3, 2023

@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
Copy link
Contributor

@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
Copy link
Member Author

rdmark commented Dec 3, 2023

@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
Copy link
Contributor

@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
Copy link
Member Author

rdmark commented Dec 3, 2023

@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 rdmark force-pushed the rdmark-issue-1392 branch from 46a9dd7 to e1278f8 Compare December 3, 2023 22:57
@rdmark
Copy link
Member Author

rdmark commented Dec 3, 2023

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.

Copy link

sonarcloud bot commented Dec 3, 2023

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
Copy link
Contributor

uweseimet commented Dec 4, 2023

@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.

@uweseimet uweseimet self-requested a review December 4, 2023 07:46
@rdmark
Copy link
Member Author

rdmark commented Dec 8, 2023

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
Copy link
Member Author

rdmark commented Dec 8, 2023

Screenshot 2023-12-08 at 10 30 58

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

@rdmark rdmark merged commit 05b9e0e into develop Dec 8, 2023
11 of 12 checks passed
@rdmark rdmark deleted the rdmark-issue-1392 branch December 8, 2023 01:38
rdmark added a commit that referenced this pull request May 1, 2024
* Correct German translation for Key

* Web UI: Rework the Attach Device section to be universal

* Web UI: Warn when working dirs are missing

* Refactor tests to use global endpoint constants

* Add fallback for unknown disk type devices

* Rearrange the index page sections

* Move Macproxy help text to admins page

* Remove image list exception for SCHD

* Show Settings button when auth is diabled

* Tweak CSS styles for both themes

* Move Eject action next to the file name, and improve UI labels
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.

Web interface issues
2 participants