-- get rid of WhichWorkbenchObject() in favor of WorkbenchControl()/WBCTRLA_* usage (allow more control) -- handled correctly selecting of more than one icon at the same time. Thanks to msteed and Javier! -- added unselecting of icons after Filer is running (to gain flexibility of different Open WB windows cases) -- updated/restructured code: more error checking, more comments, more different cases handled -- added icons from Mason (yeah!) -- readme rewritten
This one is the one i want to upload on os4depot once those who in interest can test it a little more, there is:
This small hack were a bit modified and restructured, so if one can spot in sources mistakes/bad style coding/logical issues/etc, please speak so, thanks!
@msteed Quote:
Yea, I anticipated that would be the next problem encountered, and was going to point it out if you hadn't discovered it. I'm not sure what we can do about it, though. I think we've about reached the limit of what a simple patch can do.
Thanks to Javier and his "UnSelect" idea, i implement it in a small as possible way : so when we run Filer, we unselect all selected Icons automatically by patch. Yes, it not like original WB do (original WB do keep it selected even after you click on it), but so far output is quite good as i can tell : everything works in all cases, be it WBRUN or Filer's "Open in Workbench Window", be it "Open" of workbench menu clicked when cursor over icon, or when it out of icon (from top bar), or be it simple opening of pure directories.
At least even if i hate much my own coding, i put this hack into my user-startup for now :)
Quote:
We can try to add increasingly complex patches and hacks to try to track or deduce what Workbench is up to, but we're getting to the point of diminishing returns. And all this is running inside a call to OpenWindowTagList(), so a lot of complex manipulation is probably not a good idea.
Yeah, we kind of going to the way of increasing complexity of small hack-patch , and somewhere it should stop, but so far i really want to add something of that sort before giving it a rest (of course, that not mandatory, but will be nice):
-- commoditie -- tooltype support -- when open more than one Filers, need to make them to be opened not on the same position -- when dbl-click on a partition with already opened filer, instead of opening Filer again, we need to make an opened Filer to be active instead -- some qualifier which once held down together with click on disk icon, open an original WB. Yes it possible to just click, and in Filer do "Open in WB window", but just to be more complex hack :)
All in all "complexity" is happens only when we have just a "DISK" icons opened, for all the others windows i immediately return original OpenWindwoTagList code, and for Workbench ones (but not ones which DISKs) just a little check before same original return.
- In UnSelectIcon() you don't need parenthesis for return, so just: return ISMACTION_Unselect; (return is a statement not a function so usually does not need parenthesis around argument).
- Patched_DisplayBeep() still does not check caller as suggested by @msteed in post #19. Don't know if that's needed but may be a bit less likely to break by other tasks also calling this. On the other hand if there are two beeps at the same time, one from WB and one from something else and you suppress the one from other task and beep on the WB call it's still only one beep and for the user it does not matter where it comes from as long as there aren't two beeps. So not checking for the source of call and just supress the next beep as it does now might still work unless there's some big delay between calls.
- You may be able to remove some nested ifs and indentation in Patched_OpenWindowTagList() that could make it more readable by inverting condition and return early instead of having a distant else leg and nested ifs. I.e.: if (!nw || stricmp(Caller->tc_Node.ln_Name, "Workbench") != 0 || !(nw->Flags & WFLG_WBENCHWINDOW) || (nw->Flags & WFLG_BACKDROP)) return Original_OpenWindowTagList(Self,nw, tagList);
Then the rest of the function does not need to be indented. (Indenting is already off as the if (Caller == NULL ) before it returns the same way so no need to indent what comes after. (There's also and extra space there after NULL but you could also write !Caller if you want to be even shorter.
- strncmp(strPtr+strlen(n->ln_Name)-1, ":", 1) != 0 is just strPtr[strlen(n->ln_Name)-1] != ':', no need to call a function to compare a single character. As you return from here you can also drop the else and deindent what comes after this as that will only run if the if was false and did not return here.
- Writing if ((n = IExec->GetSucc(n)) == NULL) in two lines is more readable so: n = IExec->GetSucc(n); if (n == NULL) or (!n) is more explicit. Using the value of assignment is useful sometimes like in while ((n = succ(n) != NULL) but in this case it's just makes it less readable.
- I'm not sure this which_icon counting is fail safe and can't break if you select a bunch of icons and open them but while that's processing you're quick enough to select another bunch and try to open those as well (or even just remove the selection or change it). Maybe instead of counting icons you'd better save the n value (pointer in the list) and the head of selList and if you're called with the same selList then continue from last n. Or does WB return different selList each time or it will break anyway if selection is cleared and changed inbetween as selList is always the current selection? Saving the selList and checking it did not change seems to be better but I don't quite know how this works so I may be wrong.
All in all "complexity" is happens only when we have just a "DISK" icons opened
True, the patch has minimal impact for anything other than Workbench windows.
Quote:
if one can spot in sources mistakes/bad style coding/logical issues/etc, please speak so
- For the comment "// a bit of error checking to avoid unexpected crashes" I'd add "if no icons are selected"
- In the loop where GetSucc() is called 'which_icon' times, you should check for it returning NULL each time and if so call Original_OpenWindowTagList(), just in case the number of selected icons changed unexpectedly.
- Any time you call Original_OpenWindowTagList() and return after obtaining the selected icon list you need to free the list first, otherwise you leak memory.
Quote:
once those who in interest can test it a little more
- Double-clicking the WB2Filer icon doesn't work, because there's no way to enter the path to Filer. Adding a tooltype to hold the path to Filer will fix that, but until then the icon should be set to run the program from the shell and not from Workbench.
- Incidentally, if you've run Filer previously the path to Filer can be as simple as APPDIR:Filer, or just Filer if APPDIR: is in your shell path. In fact, you might make that the default if no path to Filer is entered.
- The icon for WB2Filer.c has Multiviewer:Multiviewer as the default tool, so those without Enhancer installed will get an error when they double-click it.
Quote:
but so far output is quite good as i can tell : everything works in all cases
Double-click a disk icon to open a Filer window for that disk. Now shift-single-click several other disk icons to select them all. Select the Filer window, then Lister->Workbench Folder. Another Filer is opened for the first of the icons you selected, and all the icons remain selected. Select Lister->Workbench Folder again, and another Filer is opened for the second icon you selected. You can repeat this for as many icons as you selected. Only when there is a Filer for every selected icon do the icons become unselected.
This is also a place where not checking for NULL in the GetSucc() loop can cause a crash. Double click a disk icon to open a Filer. Now shift-single-click two disk icons to select them. Use Lister->Workbench Folder from Filer to open a Filer for the first of the two selected icons. which_icon is now set to 1, to process the second icon in the list next time. Now click the Workbench backdrop to deselect all the icons, then single-click to select only one icon. Use Lister->Workbench Folder again. Boom. Or at least, sometimes boom. Sometimes the patch just stops working.
So not checking for the source of call and just supress the next beep as it does now might still work unless there's some big delay between calls.
That's what I concluded, and why I didn't press further for checking the caller.
Quote:
Or does WB return different selList each time
My assumption is that that's the case, or at least could be the case. I imagine the contents of the list (the path names) are the same each time, as long as the selected icons remain the same, but the addresses of the list nodes are likely different.
Quote:
I'm not sure this which_icon counting is fail safe
There'a always a chance the selected icons could change or increase or decrease between one call to OpenWindowTagList() and another, causing the counting to go awry. That's why you need to check the return from GetSucc() in the counting loop to make sure it's not NULL, even though you already did that the last time the patch ran. But in practice I think it's very unlikely that Workbench is going to change its mind about which icons are selected right in the middle of opening the windows. (That said, the crash bug I noted in my reply to @kas1e is an example of exactly that, though only under unusual circumstances.)
We're also assuming the order of the selected icons in the list is the same every time, so we can pick the next one in the list each time the patch is called. That seems to be the case, since the patch works.
And the more complex you get inside a seriously critical function like OpenWindowTagList(), the worse the experience for the user.
Simon
Comments made in any post are personal opinion, and are in no-way representative of any commercial entity unless specifically stated as such. ---- http://codebench.co.uk
And the more complex you get inside a seriously critical function like OpenWindowTagList(), the worse the experience for the user.
I think you are right, need to fix remain bugs and leave it as it.
@khayoz Quote:
WB2Filer is standard in my setup now, I run it from User-Startup. Thank you! And of course big thanks to Björn Hagström for Filer!
Yeah, Filer good for sure.
@balaton, msteed
Thanks for all the suggestions/notes : deal with everything, but struggling with some issue left which i can't understand why it happens now. That what happens:
1. dbl-click on icon , filler runs. 2. select 2 icons on desktop 3. in the filer (while icons selected), hit "Workbench folder".
Now, we have only open one of the disks. While selected 2 of them, and i expected 2 of them to be opened. If i printf "which_icon" count right after SystemTagList(), then for this case i do have "which_icon = 0". Like, selection didn't happen correctly by some reasons when i hit in Filer "workbench folder". But hitting by dbl-click on those 2 selected icons, works fine, and open 2 of them.
There is just current source, check this out, maybe you will spot an obvious issue:
Now, we have only open one of the disks. While selected 2 of them, and i expected 2 of them to be opened.
That's because Filer only calls OpenWindowTagList() once*, since it's only opening a single Workbench window. If you do "Workbench folder" a second time you'll get the second disk opened. The "which_icon" method assumes that OpenWindowTagList() will be called once for each selected icon, which is true for Workbench, but not for Filer. Just one of those "assumption = always wrong some of the time" things.
* Technically, Filer doesn't call OpenWindowTagList() itself, it asks Workbench to open the window and then Workbench calls OpenWindowTagList(). But still only once, since Filer only asked it to open one window.
Technically, Filer doesn't call OpenWindowTagList() itself, it asks Workbench to open the window and then Workbench calls OpenWindowTagList(). But still only once, since Filer only asked it to open one window.
Right, so that what we kind of expect then and can keep it as it, right ?
But this one:
Quote:
This is also a place where not checking for NULL in the GetSucc() loop can cause a crash. Double click a disk icon to open a Filer. Now shift-single-click two disk icons to select them. Use Lister->Workbench Folder from Filer to open a Filer for the first of the two selected icons. which_icon is now set to 1, to process the second icon in the list next time. Now click the Workbench backdrop to deselect all the icons, then single-click to select only one icon. Use Lister->Workbench Folder again. Boom. Or at least, sometimes boom. Sometimes the patch just stops working.
While not crashes anymore, it still produces a strange side effect : after this combo, every click on disk icons on workbench runs in original window mode _always_ :) like it trash something somewhere and patch stop working correctly.
I.e. i just do this:
1. dbl-click icon, filer opens 2. select 2 icons on desktop, in filer hit "workbench folder" 3. close filers, deselect icons. 4. click on any icon on desktop => always original WB window.
Right, so that what we kind of expect then and can keep it as it, right ?
I'm not sure what we can do about it, so I guess we'll have to live with it. You'll just need to document the limitations in the ReadMe, so users understand that there will be some cases where the wrong thing happens. It's just a hack, after all.
Quote:
While not crashes anymore, it still produces a strange side effect
I noticed that with the previous version too, when I told the Grim Reaper to ignore the crash. Try resetting which_icon to zero when GetSucc() returns NULL; it may just be getting stuck at some non-zero value, which causes opening of a single icon to always fail.
I noticed that with the previous version too, when I told the Grim Reaper to ignore the crash. Try resetting which_icon to zero when GetSucc() returns NULL; it may just be getting stuck at some non-zero value, which causes opening of a single icon to always fail.
Almost :) At least now after this steps:
1. dbl-click icon, filer opens 2. select 2 icons on desktop, in filer hit "workbench folder" 3. close filers, deselect icons. 4. click on any icon on desktop
I have one time original WB window, all other clicks are working now. Seems resetting should be done early somehow as well. But i fear it also because of "filer run it once", and which icon counter mess a bit.
@msteed The more easy way to reproduce this issue with "open one by one", is by doing this:
1. after running the patch, just select RAM icon (just one enough). 2. open shell, and type "wbrun work:"
Instead of open the "work" in usual wb window, it will open for us Ram disk in Filer, because icon is selected :) Then it unselects automatically, and next "wbrun work:" open work in usual wb window as expected.
IMHO, we need to find a proper way to somehow dirreference between actual running of OpneWindwoTagList() even more than just WB/WBBACKDROP/etc. Maybe something about "active/non active" windows ? Like:
-- is "wb backdrop window active when we do selectlist ? -- if yes, do all as before -- if no, then it mean we tried from Filer or from Shell with wbrun, so then deselect them firstly.
@msteed Do some tests, and manually checked the caller of the shell and of Filer's "workbench folder", and it is the same Workbench in each case indeed. Also, all the windows (and our workbench one, and shell one, and Filer's one) are of WFLG_WBENCHWINDOW type and of not of WFLG_BACKDROP. That why check is passed even when we run from shell or filer.
So or we need to find a flag which only workbench use and no other windowss (then it will be easy right after mass selection happens and stays as it, to check if it anything than wb, and if yes, then skip it and open original window instead with return).
Or, we can go another way which i checked now : we can take AtiveWindow, and check if LeftEdge, etc are more than 0 (as WB is 0 by left Edge) then it's other windows. I checked this way now, and it works fine : even if you select icons on desktop, if you run from any other window call to OpenWindowTagList, it will open original wb window, which is what we need. And even no "unselection" happens after, which is good too.
// a bit of error checking to avoid unexpected crashes if no icons are selected.
...
But this need to think about careful, add more Edges to check, etc. Or maybe find a Flag which only desktop window have.
Alternatively, we can on the first ever run of OpenWindwoTagList(), find the first window in the whole list of windows (which is workbench one), save the address globally, and then compare each time when mass selection happens if this one is active one or not.
Edited by kas1e on 2023/12/10 13:41:03 Edited by kas1e on 2023/12/11 2:33:55
Can filer itself be made resident? There is an annoying delay the first time you open it.
On what hardware you have delay ? On all my real amigas i have no delay when run it (peg2, sam460, x1000 or x5000). Through there are a bit of micro-delay because it throws on sereal for every run that:
ListBrowserBase(HAS_NEWCOLUMNS): Test: 53.38 Has: 53.79 <- AVAILABLE
LayoutBase(HAS_NEWLAYOUT): Test: 53.0 Has: 54.10 <- AVAILABLE
BitMapBase(HAS_ALPHABITMAP): Test: 53.6 Has: 53.9 <- AVAILABLE
GraphicsBase(HAS_COMPOSITION): Test: 53.0 Has: 54.252 <- AVAILABLE
InitColors
But that happens for every run. Seems when Orgin upload latest version forget to disable debug output, but that anyway give a very little micros-pause, dunno why you have noticeable delay ..
@kas1e This is getting readable enough now (that's important because it's easier to find bugs in clean code than in one that's hard to read) but some more could be done for pre07 you've sent, all in Patched_OpenWindowTagList() func.
- The order of checking things at the beginning seems a bit random. Checking the func parameters first could save a call to FindTask when not needed, so try:
if (!Caller || stricmp(Caller->tc_Node.ln_Name, "Workbench") != 0) { return Original_OpenWindowTagList(Self,nw, tagList); } which seems like a more logical grouping of these checks.
- You could use a bit more descriptive variable names, e.g. strPtr -> icon_name, ready_string -> command_line or something that better show what they are used for.
- When you have a name for something then you can use it:
char *strPtr = n->ln_Name; if(strPtr[strlen(strPtr)-1] != ':') { and also later in asprintf, if you rename it to icon_name it's even clearer.
Also might better check string exists and long enough so: char *icon_name = n->ln_Name; if (!icon_name || !icon_name[0] || icon_name[strlen(icon_name)-1] != ':') may be safer (!icon_name[0] checks that name is not the empty string, i.e. just '\0' and we actually have a char to compare).
- You have the code fragment:
IWorkbench->WorkbenchControl(NULL, WBCTRLA_FreeSelectedIconList,selList, TAG_DONE); selList = NULL;
repeated several times. This may cause problems if you update one of the places but forget one and also makes it less readable so you could either #define a macro like FREE_SELLIST for these lines or put it in one place at the end after a label like error: and goto there from the other places. Since the normal exit also has these lines followed by a return just retuns NULL instead of original func you could add some flag like bool ok = FALSE; at the beginning then at the end return ok ? NULL : Original_OpenWindowTagList(Self, nw, tagList); (ot can write that as if () else if you prefer more explicit code, then only set ok to TRUE when you've run the command. If the above made no sense I mean:
bool ok = FALSE;
if (!n) goto end; ... // No more errors ok = TRUE; disable_flash_beep = TRUE; IDOS->SystemTags(); ...
end: IWorkbench->WorkbenchControl(NULL, WBCTRLA_FreeSelectedIconList,selList, TAG_DONE); selList = NULL; return ok ? NULL : Original_OpenWindowTagList(Self, nw, tagList);
at this point you can also drop selList=NULL as that's a local so won't exist after return so error handling is done at one place and ensured that it's the same everywhere, not missed anywhere.
Can filer itself be made resident? There is an annoying delay the first time you open it.
On what hardware you have delay ? On all my real amigas i have no delay when run it (peg2, sam460, x1000 or x5000). Through there are a bit of micro-delay because it throws on sereal for every run that:
ListBrowserBase(HAS_NEWCOLUMNS): Test: 53.38 Has: 53.79 <- AVAILABLE
LayoutBase(HAS_NEWLAYOUT): Test: 53.0 Has: 54.10 <- AVAILABLE
BitMapBase(HAS_ALPHABITMAP): Test: 53.6 Has: 53.9 <- AVAILABLE
GraphicsBase(HAS_COMPOSITION): Test: 53.0 Has: 54.252 <- AVAILABLE
InitColors
But that happens for every run. Seems when Orgin upload latest version forget to disable debug output, but that anyway give a very little micros-pause, dunno why you have noticeable delay ..
It's just the first time it opens, it takes a bit to load.
@All Through i am not sure about if it enough to just check LeftEdge of ActiveWindow != 0 to distinguish between main desktop and other windows , but at least the only situation when it will not work, is when user will move the window exactly on the 0 and somehow keep selected some icons. Probably will not happen, or so rare, that can be left as it ?
Of course, i can get the size of the workbench desktop, and compare with another Edge, but then, it will add unnecessary complexity IMHO. And actually situation when someone will select more than one icon, but not clicking on, and then running shell or filer, and specially move it to the edge 0 of desktop (while keep icons on selected), probably will be never happens. At least it surely better to have it as very rare corner case (and note in readme about) in compare with adding more complexity and have other bugs with.
@kas1e Check for example ActiveWindow->BorderTop instead of LeftEdge? Doesn't work if someone changes the workbench root window from backdrop/borderless to a normal window, but in that case your LeftEdge check doesn't work either.
News feed
OS4 Feedback forum
OS4Depot Feedback forum
Amigabounty forum
OpenAmiga forum
AmiCygnix forum
ABC shell forum
AmiKit forum
Cinnamon Writer forum
CodeBench forum
Digital Universe forum
Dopus 5 forum
E-UAE forum
Gnash forum
Ibrowse forum
JAmiga forum
Odyssey forum
OWB forum
Qt forum
SmartFileSystem forum
Timberwolf forum
TouchDevice forum
TuneNet forum
Unsatisfactory Software forum
Donate
