@techee requested changes on this pull request.

I think it looks good functionality-wise, I have just suggested to rename some 
things to make them clearer (at least to me).

> I was unable to reproduce the issue you are talking about with commit 
> https://github.com/geany/geany-plugins/commit/33984abc9bd24bbf03db3ac517eb45ca2c7e6b7f,
>  let me know if the issue persists.

When you call `prepare_fold()` the cursor is moved above it so you don't see 
it. But you can reproduce it with `cmd_close_fold_all()` which currently 
doesn't call `prepare_fold()` (but it should) and it doesn't collapse the fold 
where the cursor is placed - see the comment below.

It would be best if you rebased this PR on top of current geany-plugins master 
as there's a merge conflict in the Makefile.

> + * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, 
USA.
+ */
+
+#include "cmds/fold.h"
+#include "utils.h"
+
+/* fold command does not depend on state of parents */
+#define FOLD_NONE              0
+/* fold command depend on state of parents */
+#define FOLD_PARENT            1
+/* fold command depend on state of contracted parent if it exists */
+#define FOLD_CONTRACTED                2

I was super-confused by the naming. I's suggest the following:
* `FOLD_NONE` -> `GOTO_NEAREST_PARENT`
* `FOLD_PARENT` -> `GOTO_TOPMOST_PARENT`
* `FOLD_CONTRACTED` -> `GOTO_CONTRACTED_PARENT`

plus the corresponding  comment changes.

> + * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, 
USA.
+ */
+
+#include "cmds/fold.h"
+#include "utils.h"
+
+/* fold command does not depend on state of parents */
+#define FOLD_NONE              0
+/* fold command depend on state of parents */
+#define FOLD_PARENT            1
+/* fold command depend on state of contracted parent if it exists */
+#define FOLD_CONTRACTED                2
+
+static gint prepare_fold(CmdParams *p, gint filter)

Rename to `goto_above_fold()` and the `filter` parameter to `type`.

> +void cmd_close_fold_child(CmdContext *c, CmdParams *p)
+{
+       gint line = prepare_fold(p, FOLD_PARENT);
+       if (line != -1)
+               SSM(p->sci, SCI_FOLDCHILDREN, (uptr_t) line, 
SC_FOLDACTION_CONTRACT);
+}
+
+
+void cmd_open_fold_all(CmdContext *c, CmdParams *p)
+{
+       SSM(p->sci, SCI_FOLDALL, SC_FOLDACTION_EXPAND | 
SC_FOLDACTION_CONTRACT_EVERY_LEVEL, 0);
+}
+
+
+void cmd_close_fold_all(CmdContext *c, CmdParams *p)
+{

This will need `goto_above_fold(p, GOTO_TOPMOST_PARENT);` otherwise the fold 
where the cursor is won't get contracted because of what I mentioned before.

> +             ;       /* this fold point is contracted and filter == 
> FOLD_CONTRACTED */
+       else if (filter != FOLD_NONE)
+       {
+               gint prev_line = line;
+               while (prev_line != -1)
+               {
+                       prev_line = SSM(p->sci, SCI_GETFOLDPARENT, prev_line, 
0);
+                       if (prev_line != -1)
+                       {
+                               line = prev_line;
+                               if (filter == FOLD_CONTRACTED && ! SSM(p->sci, 
SCI_GETFOLDEXPANDED, line, 0))
+                                       break;
+                       }
+               }
+       }
+

Remove extra empty line.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1327#pullrequestreview-2072149642
You are receiving this because you are subscribed to this thread.

Message ID: <geany/geany-plugins/pull/1327/review/[email protected]>

Reply via email to