Pass folders from filename_format to upload dialog #2556
Conversation
| m_save_recent_path = false; | ||
| } else | ||
| recent_path += path.filename().wstring(); | ||
|
|
There was a problem hiding this comment.
if there is a slash (forward or backward), replace the backslashes with forward slashes then set that as the "recent_path". Also, the "recent_path_len" is updated so that only the filename portion is highlighted when saving. Saving the recent path to config is disabled (i figured it could get confusing having the folder path still appear after removing it from filename_format). If there are no slashes, the normal code is used.
There was a problem hiding this comment.
only the filename portion is highlighted
This is great 👍.
Regarding the overall logic, refer to my previous comments — I prefer to keep it simple. We should support users specifying sub-folders in the filename_format only, instead of remembering the recent sub-path, as that could create issues for users with multiple printers.
Let me know what you think.
There was a problem hiding this comment.
I would suggest we just support forward slash defined in
filename_format,
Only supporting forward slashes would make things less complex and doesn't fit the format in general. Completely agree.
so that it works for both export and upload logic
I don't completely understand this statement. It reads to me like you want the folders specified in filename_format to be passed during export gcode file as well. I don't feel that folder names should be automatically added to exported files where the user is explicitly selecting where to save the gcode file on the FS rather than a relative directory on another server/system.
We should support users specifying sub-folders in the filename_format only, instead of remembering the recent sub-path
so remove the recent sub-path function from the upload form?
There was a problem hiding this comment.
I don't completely understand this statement. It reads to me like you want the folders specified in filename_format to be passed during export gcode file as well. I don't feel that folder names should be automatically added to exported files where the user is explicitly selecting where to save the gcode file on the FS rather than a relative directory on another server/system.
That's right, basically we respect user's setting in filename_format. if a subfolder is specified in it, we will pass the subfolder to export/upload. Same logic applied to both of them, it's also consistent with the current logic. The final export path is user selected parent folder through file dialog + filename_format. So the logic remain unchanged, but now the filename_format can specify subfolder with your PR.
e.g. filename_format is a/b/somename.gcode, and user selected d:/gcode_files in export dialog, then we will export the file to d:/gcode_filesd/a/b/somename.gcode
Hope that clarifies the confusion.
so remove the recent sub-path function from the upload form?
Yes
There was a problem hiding this comment.
I feel as though that will be super confusing to the end user when they select a specific folder and it ends up creating new folders. Also, since the dialog is for saving a file (not selecting a folder) the save dialog allows for changing the name, so that would be another edge case to deal with while saving. The only time I could see it not confusing and where it possibly could be added would be when exporting to an external drive. At that time, the user is not explicitly defining exact spot where to save.
There was a problem hiding this comment.
I see your point, it also makes sense.
But it would be equally confusing if not more that if the user set subfolder in filename_format but it only applies to certain scenarios(upload here).
There was a problem hiding this comment.
After giving it some more thought, I believe we should treat the filename_format as-is, meaning it shouldn't support subfolders. However, when it comes to uploading, having a recent subfolder for each printer would be ideal.
Orca support per printer settings. refer to:AppConfig::get_printer_setting() etc
There was a problem hiding this comment.
I have to say I am strongly against doing that. How would you feel about adding a new field for this function then. If the printer config is a physical printer, then show an option for "upload folder" or something down that line?
There was a problem hiding this comment.
After giving it some more thought, I believe we should treat the filename_format as-is, meaning it shouldn't support subfolders. However, when it comes to uploading, having a recent subfolder for each printer would be ideal.
Orca support per printer settings. refer to:AppConfig::get_printer_setting() etc
should it be stored like the current recent var (as in it is done automatically) or should we add a field for the user to specify (under connection settings or printer config)?
|
I am going to close this. I want to try and adapt the panel used for BBL printers to replace this dialog. Maybe a new field can be added during that. |
passes forward and backward slashes to allow folders to be set on a per profile basis. This only applies to the the physical printer upload function.
fixes #2354