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

Keep order of project files #197

Merged

Conversation

HeX0R101
Copy link
Contributor

Project files keep their tab positions in the IDE.
Additionally I've added the project name to the Project tab (shown as: Project [Name], instead of just Project)

More or less that whole feature request from here:
https://www.purebasic.fr/english/viewtopic.php?t=66119

@HeX0R101
Copy link
Contributor Author

ouch, I might have done something wrong, I don't want to merge SIX commits for sure

@tajmone tajmone added 💡 enhancement New feature or request ⚙️ PB IDE PureBasic IDE labels Jan 25, 2022
@tajmone
Copy link
Collaborator

tajmone commented Jan 25, 2022

Project files keep their tab positions in the IDE.

That's a missing feature I always wanted — it's annoying to have to manually reorder files in the IDE by dragging tabs manually. The IDE should honour the files order found in the project file. Well done.

ouch, I might have done something wrong, I don't want to merge SIX commits for sure

You only need to squash the pull-requests commits in your PR branch into one, and then force push the branch again; this will fix the PR:

If things go wrong (locally) you can always hard reset you local PR branch (Keep_Order_Of_ProjectFiles) to its pushed version on origin, which acts as a backup.

@HeX0R101
Copy link
Contributor Author

HeX0R101 commented Jan 25, 2022

It is still odd, in that squashed commit are still all my previous commits, which have been already merged into purebasics devel branch.
Oh, my remote fork doesn't include the latest updates, that would explain it.
If I do a fetch & merge, will that sync it?

Answer: Yes it will ;)

@tajmone
Copy link
Collaborator

tajmone commented Jan 26, 2022

It is still odd, in that squashed commit are still all my previous commits, which have been already merged into purebasics devel branch.

Make sure you only squash the commits pertaining this PR, and leave previous one (i.e. those already merged) as they are.

Oh, my remote fork doesn't include the latest updates, that would explain it.

Yes, seems likely. Before creating a new PR branch you always need to:

  1. Fetch the devel branch from upstream remote.
  2. Merge into your fork's devel branch.
  3. Create the new PR branch from the up-do-date devel

If I do a fetch & merge, will that sync it?

No, fetch and synch, as described above, then rebase your PR branch on devel (your local devel should be synched after the above steps).

That should solve it, even if you squashed previous commits (hopefully, unless there are new conflicts), since contents that were already present in previous PR would be simply ignored after the rebase.

Usually rebasing is a fairly safe operation, but it might require solving conflicts at times (choosing either --ours or --theirs, which in a rebase means mean the rebased-upon and rebased-into branches, respectively).

In general, keeping one feature per commit helps making rebase operation cleaner, since it's easier to track what changes are expected in each commit.

@HeX0R101
Copy link
Contributor Author

The problem was, I made both PRs at once, but pushed just one and wanted to wait until it gets merged.
Then I seem to have forgotten a step (git and me... you know that story :))
I followed your instructions, and the changed code lines look fine, but there are still 2 commits, although it was only one.
Is that o.k. now, or do you want me to do something else?

@tajmone
Copy link
Collaborator

tajmone commented Jan 26, 2022

I followed your instructions, and the changed code lines look fine, but there are still 2 commits, although it was only one.

That's because you merged with fast-forward which always creates a commit, unlike non-fast-forward (--no-ff) merging, but unfortunately the former is the default (even on GitHub WebUI). You can tell from the automatic commit message "Merge branch XXX into", which was created by Git.

Is that o.k. now, or do you want me to do something else?

If you could squash together the last two commits (9447687 and cc7e8f9) it would help keep a clean history.

@SicroAtGit
Copy link
Contributor

SicroAtGit commented Jan 26, 2022

Before creating a new PR branch you always need to:

1. Fetch the `devel` branch from `upstream` remote.

2. Merge into your fork's `devel` branch.

3. Create the new PR branch from the up-do-date `devel`

Or you add fantaisie-software to the remotes:

$ git remote add fantaisie-software https://github.com/fantaisie-software/purebasic
$ git fetch --all

Then you can create a new feature branch from the fantaisie-software/devel branch directly:

$ git checkout --no-track -b new_feature fantaisie-software/devel
Switched to a new branch 'new_feature'

And when the fantaisie-software/devel branch has new commits, you can rebase the feature branch like this:

$ git checkout new_feature
$ git rebase fantaisie-software/devel

That's because you merged with fast-forward which always creates a commit, unlike non-fast-forward (--no-ff) merging, but unfortunately the former is the default (even on GitHub WebUI). You can tell from the automatic commit message "Merge branch XXX into", which was created by Git.

It's the other way around: fast-forward does not create a merge commit:

--ff, --no-ff, --ff-only

Specifies how a merge is handled when the merged-in history is already a descendant of the current history. --ff is the default unless merging an annotated (and possibly signed) tag that is not stored in its natural place in the refs/tags/ hierarchy, in which case --no-ff is assumed.

  • With --ff, when possible resolve the merge as a fast-forward (only update the branch pointer to match the merged branch; do not create a merge commit). When not possible (when the merged-in history is not a descendant of the current history), create a merge commit.

  • With --no-ff, create a merge commit in all cases, even when the merge could instead be resolved as a fast-forward.

  • With --ff-only, resolve the merge as a fast-forward when possible. When not possible, refuse to merge and exit with a non-zero status.

Source: $ man git-merge

@HeX0R101
Copy link
Contributor Author

Hmm... from my point of view this should be fine now, why did it fail?
Where can I see that?

My main problem was a completely scrumbled Devel branch, this here helped to get it back to normal:
https://gist.github.com/seLain/8bdf12c7196f3fccafdf067dec2696b2

@SicroAtGit
Copy link
Contributor

SicroAtGit commented Jan 26, 2022

@HeX0R101

$ sh validate.sh 

===========================================
Checking PureBasic Sources for IDE Settings 
===========================================
/// Test Passed ///

==================================================
Checking PureBasic Sources for Trailing Whitespace 
==================================================
/// Test Passed ///

================================================
Validating Code Styles via EditorConfig Settings
================================================
Using:
* Node.js v16.13.1
* EClint v2.8.1.

~~~ ERROR! ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The following files didn't pass the validation test:

PureBasicIDE/MacExtensions.pb

Run ECLint locally for detailed information about the problems.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/// Aborting All Tests ///

You can also see the above output if you click at the very bottom of the PR page on the Travis error message on Details and then on The build in the opening page.

$ eclint check
PureBasicIDE/MacExtensions.pb
    01:01 ❌ expected charset: utf-8-bom
                                         (EditorConfig charset https://goo.gl/hK94EO)

@HeX0R101
Copy link
Contributor Author

HeX0R101 commented Jan 26, 2022

interesting, I didn't touch that file at all
I think Fred has to fix that, he uploaded the MacExtensions.pb without any BOM

@SicroAtGit
Copy link
Contributor

SicroAtGit commented Jan 26, 2022

Yes, it's @fantaisie-software's last commit 4d20f77 that triggers the problem. Yes, the fix should not be a part of this PR.

@tajmone
Copy link
Collaborator

tajmone commented Jan 27, 2022

Yes, it's @fantaisie-software's last commit 4d20f77 that triggers the problem. Yes, the fix should not be a part of this PR.

I have created another version of the validation script which only validates the files which are actually affected by a commit or PR, but the problem is that it becomes too slow with medium to big projects (and with this repository, it's simply unusable).

I'm waiting for a new tool to be usable in production, replacing EClint, which should be up to task, but as of yet it still has a bug. Once the tool is ready, I'll update the validation script and it should be more precise and faster.

But then, the problem here is that no commits or PRs that fail the validation test should be merged into devel, otherwise they propagate the failure on other PRs and commits, which vanquishes the benefits of a CI build altogether.

@SicroAtGit
Copy link
Contributor

But then, the problem here is that no commits or PRs that fail the validation test should be merged into devel, otherwise they propagate the failure on other PRs and commits, which vanquishes the benefits of a CI build altogether.

Yes, this has happened several times now. It would be better if @fantaisie-software would also create PRs so that his changes are also checked with Travis. That way others could give feedback as well. Or, if he wants to keep committing directly to devel, he should at least check his changes locally with the validate.sh script beforehand.

@SicroAtGit
Copy link
Contributor

This time, I simply wanted to rebase my fork with devel (after Fred fixed the BOM thing), but git told me, there is a mismatch with the PureBasicIDE.pbp and didn't let me rebase, although *.pbp files are part of .gitignore. I don't understand, why an ignored file type prevents me to do such a simple thing like rebasing?

The rules in .gitignore only affects files, which are not already tracked by git.

The rule to ignore *.pbp files was added after the file PureBasicIDE.pbp was committed, so git keeps tracking the file:

$ git ls-files | grep .pbp$
PureBasicIDE/PureBasicIDE.pbp

You can still rebase by telling git to ignore the changes of the tracked file:

$ git checkout pr-branch
$ git update-index --assume-unchanged PureBasicIDE/PureBasicIDE.pbp
$ git rebase fantaisie-software/devel
$ git update-index --no-assume-unchanged PureBasicIDE/PureBasicIDE.pbp

Or discard the changes of the file PureBasicIDE.pbp first before rebasing, if they are not important for you:

$ git checkout pr-branch
$ git restore PureBasicIDE/PureBasicIDE.pbp
$ git rebase fantaisie-software/devel

@HeX0R101
Copy link
Contributor Author

Thanks, I deleted my post, because I thought this might be the wrong place to argue about git in general.
Anyway, thanks for the explanation, much appreciated!

;Tab can be moved by customer, therefore going through all open Tabs to find it. Maybe there is a better solution, but it works
For i = 0 To CountTabBarGadgetItems(#GADGET_FilesPanel) - 1
If GetTabBarGadgetItemText(#GADGET_FilesPanel, i) = Language("Project", "TabTitle") + " [" + OldProjectName$ + "]"
SetTabBarGadgetItemText(#GADGET_FilesPanel, i, Language("Project", "TabTitle") + " [" + ProjectName$ + "]")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to apply the change with the [project name] also in line 568 of ProjectManagement.pb (that code is called when preferences like language have changed)

That line also shows how to figure out the Tab index of the project tab without a loop and comparing element names. I think this would be a more elegant solution for this update here as well.

@t-harter t-harter merged commit 6304d7b into fantaisie-software:devel Sep 10, 2022
@HeX0R101 HeX0R101 deleted the Keep_Order_Of_ProjectFiles branch September 10, 2022 12:01
fantaisie-software pushed a commit that referenced this pull request Nov 28, 2022
Co-authored-by: HeX0R <h3x0r@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 enhancement New feature or request ⚙️ PB IDE PureBasic IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants