Include the users campaign dir in looking for campaigns

[tobylane]

Fixes #2742

Describe what the proposed change does

• Adds a new function to look for a campaign file in the users campaign folder, as well as the Corsixth folder, copied from the getAbsolutePathToLevelFile function above it.
• Add the users campaign folder to the search in the custom campaigns dialog.

[Alberth289346]

Copy-paste is oh so convenient, but pretty much always a bad idea. De-duplicating such code afterwards is horribly complicated (try it a few times :) ).

Instead refactor the checking code to its own (private) function, and use it form both spots.

[Alberth289346]

Tried this patch. It fails because I have a mazes directory in the Campaigns directory. Inside the mazes directory are the campaign files.

After copying those into the Campaigns directory, the campaign is found and started.

How to organize campaign files is a different problem from scanning the Campaigns directory, and as such, the patches solves the reported problem.

This problem should then probably be considered in a next issue, since I am pretty likely not the only person that wants to organize their data differently compared with throwing it all in one directory.

[Alberth289346]

Apparently, @lewri has previous art with respect to directories in #2209

[tobylane]

Refactored the two almost identical functions. Added an information message and detail in console for duplicate campaign names. The UIInformation window is set to be on top, but is below the campaigns dialog.

[tobylane]

Moved the warning to the dialog, where it's visible. Rebased.

I added and removed another commit, but in writing and rebasing that, I don't like it. I think we should be passing around the full path of the file the user picks, rather than just the name then try to find the right one later.

[Alberth289346]

I added and removed another commit, but in writing and rebasing that, I don't like it. I think we should be passing around the full path of the file the user picks, rather than just the name then try to find the right one later.

I agree that passing around a full path makes life much convenient and less error prone, go for it !

You could also see it as having 2 needs. One need is the program finding the file again that you mentioned above. The other need is showing the user what they get, and a full path may not the best information for that.
Of course the latter can be expanded with a teaser text, some pictures, etc etc :p

I can live without the user part, but it may be useful not to make that impossible to add.

[tobylane]

Full paths are used where the file will be used straight away, filenames and folder of parent file are used where the file might be opened later, eg on another computer. All subfolder (one deep) are checked for campaign files, but only the subfolder with the campaign file will be searched for the level file, and same with the level file dir and map file. I'm intending that a whole campaign can be in its own folder, and can use files outside of it. A map reused in a second campaign would be duplicated in the second folder.

[Alberth289346]

If you need to make a backup, you may be doing things in the wrong order or you may have made a premature "jump" to the conclusion (as in you computed "the" answer, but for some reason it's not always "the" answer, and you need to rescue some values).

A different possibility is that the variable has in a sense "several" values that do not belong together just yet. (ie the values were merged too soon).

Here, it looks like you try too soon to create the level_info triplet values (level_file, name, and map_file). If you use separate variables for them, and then after everything is decided, you construct the triplet, I think the problem disappears.

To some extent this may also happen because we tend to compute everything in one function even if some parts are just different attempts to compute the same thing. Splitting it into more smaller functions may kind of "automatically" solve such problems, because bottom-level stuff is then done inside the child functions, and here you only worry about the higher level coordination of what function to call when.
Heaps to gain there in our code :)

[Alberth289346]

This modifies search_paths, which may not be expected by the caller (and the Lua doc also doesn't warn about that).

This function is doing 2 different things. It decides order of the paths to search (for one special case only), and it does the search.

The method name only claims it does a search. So there are 2 ways here to solve it.

1. Do what you already do here, but without messing up search_paths.
2. Move setting up the order of the search paths out of this function.
3. Rename the function to something wider (but imho that is not a good idea, deciding order and doing searching are different things).

It's a weird that you have search-paths in the order of visiting, but than apparently you need to fix the first search. That points at the search path construction code which is thus apparently doing something wrong.
In such cases, fix the code that is broken rather than adding some additional fixing code for special cases. If you do that long enough, nobody knows anymore what is supposed to happen where.

Code that is doing some job should completely do that job, so other code doesn't need to bother about it.

[tobylane]

Chose 2, especially as there's now one case of adding two paths.

[Alberth289346]

Not your fault probably, but if you look at this code you'll notice that 662 to 665 and 667 to 670 do the exact same thing, except for different pieces of text.

By encapsulating the 4 lines into a function and giving it contents and "Briefing" resp "Debriefing" you can move the 10 lines of details out of this function (for as far as I can see now), and replace it by 2 calls to the new function.

By doing that more, you get higher-level functions that handle bigger steps in the code without getting several hundreds of lines long. Reading and understanding such functions is much easier too, as you never even see all the little details that are hidden in the called functions.

[Alberth289346]

In general, looking down for the next matching elseif, else, or the end line is not trivial, especially if they code in-between is long.
For this reason prefer handling the shorter alternative first. An alternative that ends the function is also a good option, since it avoid handling multiple cases at the same time and/or doing the same test more than once (that doesn't quite happen here at first sight).

At a higher level, several lines look like they are generic. Eg the "test presence of a file" lines and "read the data lines if the file exists", and the "decompress if compressed" lines are used elsewhere too in the exact same way, and are thus good candidates to move out of this function.
Not sure if "read a level file and throw an error if missing" is common too, but it could be.
Mostly this is again "don't do everything in one function, move lower level details out of the way".

Not sure if you should do that now. I can live with not doing this.

[Alberth289346]

Left side has 3 variables, right side has 2 values.
As it is now, it can be easily seen as a missing 3rd value.

If it is not, split the variable declarations so it is clear that it's not a mistake.

[Alberth289346]

Avoid using and setting variables that exist outside the function. It makes understanding and debugging a mess. It also makes it impossible to move this local function out of this code without major headaches and/or re-writing.

In my view, Lua allowing this is a major wrong design decision.
It may "simplify" small scripts, but at our scale the benefit of less input/output parameters is a non-problem compared to difficulties in understanding code structure, and what variables gets used and assigned where.

[Alberth289346]

One way to avoid these problems is to only have outer-level functions. Use a _ prefix to indicate private functions then.

[tobylane]

Changed to private class functions, and class variables.

[Alberth289346]

It also returns a duplicates flag

Technically the description at line 35 isn't complete, it also checks for validity of the campaign and reports errors to the log

[Alberth289346]

You recognize duplicates while reading, and then report it here.

If you read and return every valid campaign you find, and here check the results for duplicates, you separate duplicate handling from reading campaigns.

[Alberth289346]

Eg extending to "3 similarly named campaigns X were hidden" becomes simpler then, as doesn't need to be done inter-mixed with reading and validating.

That is, doing one thing at a time often makes extending functionality tomorrow simpler.

[Alberth289346]

Console? Isn't that "Log" ?

[tobylane]

Thanks, made changes. Should I avoid this overwriting of previous commits?

[tobylane]

Chose 2, especially as there's now one case of adding two paths.

[tobylane]

Changed to private class functions, and class variables.

[Alberth289346]

Thanks, made changes. Should I avoid this overwriting of previous commits?

In general, yes. Don't use it unless there is no other way.

The idea of open source is that everybody shares and extends code with each other.

For example, I can create a patch on top of your patch (even while you aren't done yet with your patch) to fix some problem or add new functionality.
I can share my patch with you and you can merge it with your own patch.
Alternatively, if your patch gets merged before I am finished, I can submit my patch afterwards.

All the above assumes that your patch and my patch are compatible. They have a common starting git revision, a common history.
Under that condition, git allows either of us merging an external patch into our own patch.
If you force-push, there is no common history anymore, and merging other code becomes impossible. That is mostly very bad news for the person(s) not doing the force-push.

Of course, all this is only a problem when there is a need to merge code. If you are the only user of a repository there is never a problem. Here, someone could create a patch on top of your branch and force-psuh would give trouble, but the likelihood of that happening is not big. If however we would force-push in master, then everybody writing patches for CorsixTH would be affected.

Thus, the history problem for your patches is mostly a theoretical problem until you start co-operating with someone to write code with more than one person in the same branch without eg a review process like we have here.
On the other hand, if you want to do that at some point in time, it doesn't hurt to get used to the idea that history shouldn't be destroyed if it is avoidable.

[Alberth289346]

I think the patch is ok