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

Refactor setLock() and getLock() #9866

Merged
merged 13 commits into from Jun 15, 2021
Merged

Conversation

danielb987
Copy link
Contributor

@dsand47
I have earlier added the methods getLock() and setLock(), but I have never used them. But the way these was implemented wasn't so good. So I have refactorized these methods by moving them from the interface jmri.jmrit.logixng.Base to jmri.jmrit.logixng.MaleSocket.

This PR will have a conflict with #9864 that I will resolve once #9864 is merged.

TreeEditor works with female sockets and male sockets, so TreeEditor has access to the methods getLock(), setLock() and isSystem(). The idea is that the user can lock and unlock a node and I think it would be good if that would lock/unlock all the nodes in the sub tree of that node as well.

If a male socket is system (the method isSystem() returns true), then it would be good if the user gets a dialog box asking for confirmation when he tries to edit, cut or delete a node.

The method setSystem() is intended for ImportConditional when importing system Logixs and if a JMRI tool creates internal LogixNGs for its own use.

@danielb987 danielb987 added this to the 4.23.7 milestone Jun 4, 2021
@danielb987 danielb987 self-assigned this Jun 4, 2021
@mergeable mergeable bot added the New PR Newly created PR that is less than 1 day old label Jun 4, 2021
@mergeable
Copy link

mergeable bot commented Jun 4, 2021

Thanks for the PR. Please consider adding a release note in the help/en/releasenotes/current-draft-note.shtml file.

@mergeable mergeable bot added the Needs L10N Needs localization label Jun 4, 2021
@mergeable
Copy link

mergeable bot commented Jun 4, 2021

Thanks for the PR. It includes changes to properties files, so the 'Needs L10N' label has been added"

@mergeable mergeable bot removed the New PR Newly created PR that is less than 1 day old label Jun 8, 2021
@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #9866 (7eaea3d) into master (a185c38) will increase coverage by 0.1%.
The diff coverage is 41.8%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #9866     +/-   ##
===========================================
+ Coverage      51.7%    51.7%   +0.1%     
- Complexity    64220    64221      +1     
===========================================
  Files          5112     5112             
  Lines        446298   446379     +81     
  Branches      64477    64518     +41     
===========================================
+ Hits         230682   230750     +68     
+ Misses       192837   192820     -17     
- Partials      22779    22809     +30     
Impacted Files Coverage Δ
java/src/jmri/jmrit/logixng/Base.java 77.3% <ø> (-2.1%) ⬇️
java/src/jmri/jmrit/logixng/MaleSocket.java 65.0% <ø> (ø)
...ri/jmrit/logixng/actions/AbstractAnalogAction.java 93.4% <ø> (-0.7%) ⬇️
...i/jmrit/logixng/actions/AbstractDigitalAction.java 100.0% <ø> (ø)
.../logixng/actions/AbstractDigitalBooleanAction.java 100.0% <ø> (ø)
...ri/jmrit/logixng/actions/AbstractStringAction.java 93.4% <ø> (-0.7%) ⬇️
.../logixng/expressions/AbstractAnalogExpression.java 88.3% <ø> (-1.2%) ⬇️
...logixng/expressions/AbstractDigitalExpression.java 100.0% <ø> (ø)
.../logixng/expressions/AbstractStringExpression.java 88.3% <ø> (-1.2%) ⬇️
...t/logixng/implementation/AbstractFemaleSocket.java 75.4% <ø> (-1.0%) ⬇️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a185c38...7eaea3d. Read the comment docs.

@coveralls
Copy link

coveralls commented Jun 11, 2021

Coverage Status

Coverage increased (+0.01%) to 56.862% when pulling 7eaea3d on danielb987:lock_unlock into a185c38 on JMRI:master.

@danielb987
Copy link
Contributor Author

@dsand47
I have implemented Lock/Unlock in this PR.

If an item is locked, a lock icon is shown before that node.

If the user selects "Lock" on an item, that item and all its descendants are locked.
If the user selects "Unlock" on an item, that item and all its descendants are unlocked.

An item can be edited, enabled and disabled if it's not locked.

An item can be added if the parent is not locked. An item can be cut or removed if it's not locked and its parent is not locked.
.

This may seem difficult, but my idea is:

  • If the item is locked, it should not be possible to change that item (it's settings, username, local variables, ...) or to remove it (cut, remove).
  • If the parent is locked, it should not be possible to reorder its children or to add or remove its children.

I have added a warning dialog if the user tries to edit/cut/remove a system item. You can test this by changing system="no" to system="yes" in the panel file. With system, I'm referring to things like Logix RTX and SYS, that is items that are created and maintained by JMRI itself.

@danielb987 danielb987 changed the title WIP - Refactor setLock() and getLock() Refactor setLock() and getLock() Jun 14, 2021
@danielb987
Copy link
Contributor Author

@dsand47
This PR is ready for review.

Copy link
Contributor

@dsand47 dsand47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I assume it is ok to unlock a child.

@danielb987 danielb987 merged commit 8dccdd6 into JMRI:master Jun 15, 2021
@danielb987 danielb987 deleted the lock_unlock branch June 15, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs L10N Needs localization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants