Refactor: IE VI Position

[neilpate]

Currently there is way too much going on in the top level.

I propose to make some simple changes to tidy up some of the code into sub-VIs.

First step is to address stuff like this. Potentially leveraging the fact that there are already VIs to do setting/getting config information from LabVIEW.ini

[neilpate]

In principle, I would prefer to not see too many LabVIEW primitives on the top-level.

[neilpate]

And I would remove some of the wires that are just used for sequencing (and replace them with an error cluser).

In the end I picture just one VI outside the event structure, and it does all the initialisation stuff, and one VI at the end to do the tidy-up.

[neilpate]

My feeling is all these edits can be done in a way that completely keeps the current behaviour, so no actual change to functionality.

[crossrulz]

I have had my eye on those specific spots for some time, I just kept finding bigger things to tackle.

My vision for this, which I do have a working branch for, is to turn the position code into a class. This would have a private VI to hold the ini key(s) to read/write, and private data to hold the starting position and VI reference.

For direct replacement, this code is ready to go. However, I would like to add functionality to keep the user from updating the ini file manually and resizing the panel. This is where I am stalled, mostly due to being distracted by 1M other things.

Additionally, we should make some unit tests for this new class.

[crossrulz]

Hmmm. Do you think Editor Position is important enough to warrant its own class? I would have thought maybe that Settings/Configuration would be a better candidate.

Thinking about this more, you may be on to something. I need to investigate the initialization and everywhere the initialization is called, but the write part could easily be moved into the Write INI Token VI that runs right before the Keep In Memory VI.

[neilpate]

Should we park this until you have done your Event stuff?

[crossrulz]

No need to park this. The only real issue is lv_icon.vi. As long as we avoid touching that VI until the Event stuff is in Develop and a proper rebase can be done, things can progress.

In other words, the class, VIs, and/or some subVI can be developed in parallel. At the very least, we can form a plan of attack while the review and merging process goes through.

[neilpate]

yeah but I think this change will definitely affect lv_icon.vi

[neilpate]

We can chat about this in the call on Thursday.

[neilpate]

@crossrulz now that your changes have been merged, can I make a start on this?

[crossrulz]

Unfortunately, @svelderrainruiz accidentally broke the merge history through a series of events. Waiting for him to fix it before we can really move on anything.

(This post is another way to poke him.)

[svelderrainruiz]

Unfortunately, @svelderrainruiz accidentally broke the merge history through a series of events. Waiting for him to fix it before we can really move on anything.

(This post is another way to poke him.)

Fixed. You can see the new name of the feature branch on #233.

[svelderrainruiz]

@neilpate yes, you can make a start on that. Please go ahead and make the GitHub issue.

[crossrulz]

@neilpate,
Let's talk about how we really want to do this.

Do we even care about writing to the ini file if there was no change in position? I'm inclined to say no, especially since there are a whole bunch of other settings that are written to the ini file unconditionally. See WriteDataToLabVIEWINI.vi. If we eliminate the need to store the position value, then I have no real argument to make this a class. So making 2 new VIs makes sense: One to read the position in the ini file and set the IE VI position and another to write the position to the ini file.

I propose we put these two VIs in the ReadDataFromLabVIEWINI.vi and WriteDataToLabVIEWINI.vi.

The Read is only called on the first call of Load.vi (poorly named VI), which should occur in the initialization. It will need to be updated to have the IE VI reference passed in to be able to set the position of the IE VI. I am also extremely tempted to turn it into a Natt Sequence (https://forums.ni.com/t5/Community-Documents/Natt-Sequence/ta-p/3970582) or similar (use the INI keys in an array of strings to loop over). The idea here being a sequence loop would be a lot easier to read (eliminate a lot of long and crossing wires).

The Write is only called in the Write INI Tokens and VI Tags.vi, which is only called in the IE VI cleanup (first VI in the Flat Sequence Structure). ~~From there we have the choice to have the WriteDataToLabVIEWINI.vi accept the IE VI reference or the position. I lean towards the reference, but could easily be convinced going the other way.~~For parity with the Read, it makes a lot more sense to pass in the IE VI Ref.

Going this route will completely eliminate this code (and 2 long wires!) from the main diagram.

What do you think?

I can also do a quick class elimination of the VIs I already have implemented and get them into the feature branch for you to start from.

[svelderrainruiz]

Can i sign you both @neilpate and @crossrulz to create multiple GitHub issues for small features? I believe the thread of work on this discussion could attract a lot of people to collaborate for the first time.

By "multiple github issues" i mean as many as you can, i would even encourage you to spend more time on making the issues than actual implementation, since we had an influx of people that are waiting for good first issues.

[crossrulz]

@svelderrainruiz, if we really want to break this down into tiny steps, I see 4 issues we could create:

1. Create subVIs to set the window position based on the LabVIEW.ini file (Set Editor Position.vi) and to write the updated position to the LabVIEW.ini file (Save Editor Position.vi). I have these ready to go and can immediately put in a PR. I will also remove the current code from lv_icon.vi.
2. Add Set Editor Position.vi to ReadDataFromLabVIEWINI.vi. Optionally, refactor ReadDataFromLabVIEWINI.vi to make it easier to read, understand, and edit.
3. Add Save Editor Position.vi to WriteDataToLabVIEWINI.vi.
4. Write unit tests for Set Editor Position.vi and Write Editor Position.vi.

If we really want it broken down to this level, we can. Feels weird to me as these are super tiny, most of which will not take more than a few minutes. Would we use a single feature branch for all of these tasks and then have a single PR into develop when it all comes together?

I can get all of this created (issues and branch(es)) once we get the plan in place.

[neilpate]

I don't think this particular feature is a good thing to break into sub-issues. These all belong in the same refactor exercise, and are small changes.

[crossrulz]

I'm with @neilpate. I'll get the issue and new branch created this afternoon/evening.

[svelderrainruiz]

@neilpate @crossrulz I agree with you, this is not the right feature to do this.

We should think on other github discussions that are mature enough to create github issues from them. I am adding this to next weeks agenda.

As an example of very small changes that we could open for collaboration, take a look at this one that @dnattinger asked us to do, i think its a very good example of a small change that can have a big impact, and that was taken by the community because it was very well scoped.

[crossrulz]

Implemented with #285